fix(core): memory leak in memorized decorator #17319
Conversation
zhaoyongjie
changed the title
zhaoyongjie
force-pushed
the
fix_memory_lack
branch
from
b782f4f
to
ca710d3
Compare
zhaoyongjie
changed the title
Codecov Report
@@ Coverage Diff @@
## master #17319 +/- ##
===========================================
+ Coverage 66.40% 76.96% +10.56%
===========================================
Files 1641 1037 -604
Lines 63520 55629 -7891
Branches 6422 7608 +1186
===========================================
+ Hits 42178 42817 +639
+ Misses 19681 12562 -7119
+ Partials 1661 250 -1411
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
zhaoyongjie
force-pushed
the
fix_memory_lack
branch
2 times, most recently
from
1c611a7
to
46eeb54
Compare
zhaoyongjie
force-pushed
the
fix_memory_lack
branch
from
46eeb54
to
ddf8905
Compare
update doc wip wip fix lint
zhaoyongjie
force-pushed
the
fix_memory_lack
branch
from
ddf8905
to
88eea26
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
|
I am also observing memory leak. Any updates on this PR? |
|
@mdeshmu let me update this PR and try to resolve it. |
| @@ -349,6 +349,29 @@ def get_sqla_engine( | |||
| nullpool: bool = True, | |||
| user_name: Optional[str] = None, | |||
| source: Optional[utils.QuerySource] = None, | |||
| ) -> Engine: | |||
| cache_key = ( | |||
There was a problem hiding this comment.
Shouldn't we removed @memoized from this method?
There was a problem hiding this comment.
Also, using secret (un-hashed) material as a cache key seems a bit dangerous. Ideally, we would at least hash before using as a cache key.
There was a problem hiding this comment.
This PR has been open for a long time, I'll finish it and ping you. Thanks for the reviewing
| def __repr__(self) -> str: | ||
| """Return the function's docstring.""" | ||
| return self.func.__doc__ or "" | ||
| def wrapper_cache(func: Callable[..., Any]) -> Callable[..., Any]: |
There was a problem hiding this comment.
Looks like the new version is missing the watch kwarg functionality that allowed args to be selectively included as part of the cache key
There was a problem hiding this comment.
arguments of the function are implicit watch hash id.
|
Thanks @zhaoyongjie |
|
@zhaoyongjie can you please revisit this PR once you get time. |
|
@zhaoyongjie any update here? @rusackas IMO, Its not good for an excellent tool like Superset to have a code that causes memory leak, not fixed for an year. Can you please get this fixed ASAP. Appreciate everyone's efforts in keeping the tool top notch. Thanks. |
|
@zhaoyongjie @villebro @etr2460 @craig-rueda @dpgaspar Is it possible to fix the problem and merge it? |
SUMMARY
closes: #15132
Currently, we use
@memorizeddecorator to cache function results in memory. Obviously, the original decorator does not consider:In this PR:
functools.lru_cachewrapper replaced with originalmemorized.I usedpickle cannot serialize SQLAlchemy engine object, raise an errormemoized_funcreplaced withmemoizedwhen need to define a custom cache key.TypeError: can't pickle _thread._local objects, so we we can't use memoized_func to cache this functionfunctools.lru_cachecan't support customized cache-key, add a minor proxy function to delegateget_sqla_enginefunctionBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION