Skip to content
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

Keep the only one record per module-function in depends decorator. #38951

Conversation

DmitryKuzmenko
Copy link
Contributor

What does this PR do?

As I see the initial design of the depends decorator used set to keep the unique seet of decorated functions determined by the module, function and it's alternative implementation.

After functions become loaded with lazyloader the same function loaded twice become another object.
Also the module was replaced by frame that is also another object each time.

What we got as a result: depends decorator leaks on each lazyloader instantiation.

What issues does this PR fix or reference?

Fixes: #37938, #33890

Previous Behavior

depends uses a set of (frame, function, alternative_function) to reference the decorated functions.

New Behavior

Use dict where the key is (module_name, function_name) to keep the only unique functions.

Tests written?

No

@DmitryKuzmenko
Copy link
Contributor Author

BTW, maybe it's better to rebase it to 2016.3 branch? It's also affected.

@cachedout cachedout changed the base branch from 2016.11 to 2016.3 January 26, 2017 19:06
@cachedout cachedout changed the base branch from 2016.3 to 2016.11 January 26, 2017 19:06
@cachedout
Copy link
Contributor

@DmitryKuzmenko Yes, that is what we will want. I tried to change the base here but it did not merge cleanly at all. Could you please close this and open another PR against 2016.3? Thanks.

@DmitryKuzmenko
Copy link
Contributor Author

@cachedout will do.

@DmitryKuzmenko DmitryKuzmenko force-pushed the bugs/37938_fix_depends_decorator_memleak branch from 8749f9a to 0b18f34 Compare January 27, 2017 08:09
@DmitryKuzmenko DmitryKuzmenko changed the base branch from 2016.11 to 2016.3 January 27, 2017 08:09
@DmitryKuzmenko
Copy link
Contributor Author

@cachedout I've rebased the branch and changed the base here so there's no need to open a new PR.

@cachedout cachedout merged commit da96221 into saltstack:2016.3 Jan 27, 2017
@DmitryKuzmenko DmitryKuzmenko deleted the bugs/37938_fix_depends_decorator_memleak branch February 1, 2017 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants