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

Changed service registrations from Transient to Singleton. #52

Conversation

paulomorgado
Copy link
Contributor

What issue does this PR address?

Addresses #51

@josephdecock josephdecock merged commit f882ddc into DuendeSoftware:main Feb 26, 2024
@josephdecock
Copy link
Member

Thanks!

@josephdecock
Copy link
Member

Note: Need to document this as a breaking change in the release notes (hence the tag), because customizations that have scoped dependencies will have to be updated.

@brockallen
Copy link
Member

I'm happy to revisit this PR as a whole if customizations are more difficult due to the assumptions imposed by things being registered as singletons.

@josephdecock
Copy link
Member

josephdecock commented Apr 30, 2024

I am going to revert this change because of the potential to cause breaking changes for users. Any customization that is injecting a scoped dependency would be required to have an update, and we've decided that that is too much of a breaking change for the potential reduction in memory allocation. When this first came up in #51, I wish I would have highlighted from the start that our driving design goal here is to be extremely flexible and customizable, and one way that we facilitate that is by making our services scoped so that customizations can use scoped dependencies easily.

If anyone is experiencing performance problems or memory pressure coming from this code, we do want to hear about it, and we could potentially revisit this (especially if we hear of more practical performance issues). But for now, we need to pull this out. I believe it is technically possible to re-register the dependencies as singletons to override their lifetime as well (admittedly, a bit of a PITA, but a way to get this behavior if you really need it nonetheless).

@brockallen
Copy link
Member

Also, anyone where the memory issues are particularly important can always re-register things as singletons, yes?

@josephdecock
Copy link
Member

Yes, that should be possible.

@josephdecock josephdecock removed this from the 3.0.0 milestone Apr 30, 2024
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.

Why is AddClientCredentialsTokenManagement registering most services as transient?
3 participants