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

Disable lifetime service in TransformationContextProvider #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ItielBeeri
Copy link

Addresses #54

@olegsych
Copy link
Owner

@ItielBeeri Thanks for the PR and my apologies for the delayed response.

If I understand correctly, by returning null from InitializeLifetimeService we are making the lifetime of the TransformationContextProvider service infinite. Does that mean that the service instances created for each transformation will not be released, creating a memory leak?

If so, can we avoid the memory leak by making the TransformationContextProvider.Register add service to the IServiceContainer as a single object instance instead of a ServiceCreatorCallback?

Alternatively, can we keep the lifetime as is and avoid the object disconnected error (#54) by making the TransformationContext get the ITransformationContextProvider service every time it's needed instead of getting it once at the start of the transformation?

@ItielBeeri
Copy link
Author

@olegsych, thanks for your reply.
I thought VS anyway manages the provider lifetime to be a singleton through the VS instance lifetime, just returns different values on various transformations.
Apparently I wrong, and indeed, your both suggestions could prevent the memory leak. The later seems more robust, by avoiding retain a static instance, but it might fail if TransformationContextProvider will evolve in a way so it will depend on an internal state. What do you think?
Anyway, I'll try out both approaches and will come back soon with a better solution.

Copy link

@baywatch baywatch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please merge this into master

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.

3 participants