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

microprofile-health-128 org.eclipse.microprofile.health.HealthCheckResponse leaks #264

Closed
wants to merge 1 commit into from

Conversation

antoinesd
Copy link
Contributor

fixes #128 with multiple providers support addition
Tests also added to api to make sure multiple provider resolution works as expected

@antoinesd antoinesd requested a review from xstefank July 20, 2020 11:14
@antoinesd
Copy link
Contributor Author

Hi @rmannibucau, could you check if this PR meet your concern in #128 ?

Copy link
Member

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

api/src/test/resources/META-INF/services/dummy should probably be removed. At least I missed its use.

@rmannibucau
Copy link

Hi @antoinesd , from my understanding it just moves the issue somewhere else (basically while there is a setter, with or without a filter the issue remains). A clean solution is likely to make the factory (builder) an injectable bean which would therefore fully inherit CDI context definition in next major. Until then keeping current solution sounds like acceptable.

@antoinesd
Copy link
Contributor Author

@rmannibucau the proposed solution allows the usage of multiple implementations even impl provided in app as your mentioned in ticket #128, so I really don't get your issue. As we won't drop the service loader usage I don't see how your suggestion could be viable.

…sponse leaks

fixes eclipse#128 with multiple providers support addition

Signed-off-by: Antoine Sabot-Durand <antoine@sabot-durand.net>
@antoinesd antoinesd force-pushed the microprofile-health-128 branch from 3680d05 to 467cf6b Compare July 20, 2020 13:59
@rmannibucau
Copy link

@antoinesd the PR still leaks the impl (the provider and the predicate) and is still not aligned on the CDI container lifecycle (which is this ticket). Why serviceloader wouldn't be dropped? There is no technical reason to duplicate the impl loading.

@antoinesd
Copy link
Contributor Author

Sorry Romain, there is no mention of CDI in the initial ticket. The use case you mentioned in it was to support multiple impl (even one included in the app) which is now the case.
Regarding ServiceLoader usage it's a good practice used in a lot of specs (including CDI), so we won't drop it without a very good reason. As some people are using Health without CDI it would break a lot of usage if we should based the impl loading on CDI injection.

@rmannibucau
Copy link

@antoinesd hmm I didn't write "CDI" but that the builder should be "injected", sorry if it was not explicit enough.
Regarding ServiceLoader, the only reason most spec use it is to be independent from others, MP is not and clearly depends on CDI so only CDI must use a ServiceLoader to lookup the provider, then everything should just be beans to avoid context issues.
For cases where you don't have CDI, it is exactly the same - we did it at Geronimo were we extracted part of the impl in a "common" module to let it be used in OSGi or standalone. The main difference is the caller must define the "context"/scope and not rely on an implicit global variable which leaks if you re-execute some processes.
Another broken use case is when you start weld twice in the same JVM and want one impl per container (cause a lib depends on it).
So whatever wayt I'm thinking about it, I only see the builder as being injectable sane. For standalone case, the SPI could still be used if set by vendors or a plain new could be done of the provider but the spec does not handle or need it.

@antoinesd
Copy link
Contributor Author

Obviously, I totally missed your point and I'm not quite sure to understand what you suggest. As most MP are using ServiceLoader, I guess that the problem you raise is at the platform level and should be discussed in the MP mailing list.
In the meantime, I close this PR and move your ticket for 4.0.

@antoinesd antoinesd closed this Jul 21, 2020
@rmannibucau
Copy link

@antoinesd oki, agree it can be tackled at platform level too. What I hit was the leak between containers instances (recreating classloaders i switched of provider unintentionally in a complex setup) but the underlying issue is really the fact to not align properly on CDI using ServiceLoader (or alike) API in specs.

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.

org.eclipse.microprofile.health.HealthCheckResponse leaks
3 participants