-
Notifications
You must be signed in to change notification settings - Fork 53
Memory leak hotfix #184
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
Memory leak hotfix #184
Conversation
Tests are failing now? |
Yes you're right, what I wrote is breaking the chain. I'm going to look at it better |
875ac2f
to
9e87d6b
Compare
So, the problem is on passing the first callable to plugins, it creates a reference loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks reasonable to me.
@Nyholm do you agree, or are you worried about side effects or some other overhead resulting from this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really call the plugins in correct order?
I might be misreading something, but it seams like the order is reversed now. Do we have tests for the order plugins are called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the CS fixes, then rebase.
@GrahamCampbell is the change with the plugin chain good otherwise? |
Yeh, if you extract just the changes to the files that are needed (I think 3 files?). |
Sorted out a replacement here: #194. |
Hi,
there is a big memory leak when using the
PluginClient
with plugins.Example code to reproduce the issue:
This PR fixes it removing useless variable reference.