Skip to content

Disable lifetime service in TransformationContextProvider#55

Open
ItielBeeri wants to merge 1 commit intoolegsych:masterfrom
ItielBeeri:master
Open

Disable lifetime service in TransformationContextProvider#55
ItielBeeri wants to merge 1 commit intoolegsych:masterfrom
ItielBeeri:master

Conversation

@ItielBeeri
Copy link
Copy Markdown

Addresses #54

@olegsych
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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