-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix memory leak issue #258
Conversation
Here we are more careful about which caller's locals we use to resolve forward type references. We want the callers locals at decoration-time — not at decorator-construction time. Consider: ```py frozen_dataclass = marshmallow_dataclass.dataclass(frozen=True) def f(): @custom_dataclass class A: b: "B" @custom_dataclass class B: x: int ``` The locals we want in this case are the one from where the custom_dataclass decorator is called, not from where marshmallow_dataclass.dataclass is called.
When class_schema is called, it doesn't need the caller's whole stack frame. What it really wants is a `localns` to pass to `typing.get_type_hints` to be used to resolve type references. Here we add the ability to pass an explicit `localns` parameter to `class_schema`. We also add the ability to pass an explicit `globalns`, because ... might as well — it might come in useful. (Since we need these only to pass to `get_type_hints`, we might as well match `get_type_hints` API as closely as possible.)
@lovasoa On first glance, both PRs look good. @mvanderlee Thank you for picking up the ball. It's great to see progress on these issues. |
I currently have 2 month old baby at home, and way less free time than before. I fully trust you to take the right decisions, don't hesitate to merge this and other PRs without me :) |
Congratulations! |
Since I also suffer from the memory leak, I quickly integrated this branch into my project (50 kLOC service-based architecture) and ran my tests (5k+) against it. It looks very good, everything works as expected. Perhaps this will help you decide whether to merge this PR in the near future. |
@dairiki I think you can merge |
I've got a lot of pans in the fire at the moment. |
Merged and released as 8.6.1. Thank you @mvanderlee for getting the ball rolling! |
Fixes: #198
Rebased and cherry picked but all work is credited to @dairiki
I tried rebasing #232 but ran into a few conflicts I didn't feel comfortable resolving. So figured I'd break it out into smaller PRs that can be merged individually so we can get some progress on some of these issues.
I've tested this with
tox
on python versions 38, 39, 310, 311, 312