bugfix: clean up effectiveInjectionManager and move cleanup into fina…#6099
bugfix: clean up effectiveInjectionManager and move cleanup into fina…#6099lprimak wants to merge 1 commit into
Conversation
|
@OndroMih See this PR |
|
How does this fix the leak? Which thread local is the cause of the leak, effectiveInjectionManager or threadInjectionManagers? |
|
|
|
Looks like CI passed. There seem to be tests that check integration against weld and HK2. |
|
I tested this PR in Payara, and it works. Rest APIs work, there are no errors, and there are no longer ClassLoader leaks. |
|
I don't know enough about Jersey code base to tell you right now that all is OK with this change. I'm afraid that it might break some cases that are not covered by tests because the scenario I'm concerned about is sort of a special case - inject would need to be called twice. I'll see if there's a test that covers that scenario and will try to write a test case for it but I don't know when I will have time for that. If you want to help me, try creating a test or modify your reproducer so that inject is called multiple times, if that's possible. I would try with creating some CDI beans that contain several @Inject fields. |
|
@lprimak , do you see "Leaking injection manager found in the current thread" errors printed from the |
|
Ondro, I have tested this with a real application that has 100s of Injected fields, it works fine. Have you by any chance seen the initial issue that was filed against Payara for this? I really have done all I can here, going far "above and beyond" since I am not working for Payara, or OmniFish, etc. |
|
Hello, @lprimak I ran the reproducer https://github.com/AndrewG10i/jakartaFacesTestApp/ according to your instructions, and I see the following:
So it seems to me that the issue is only in Payara Micro and only in the patched Jersey version in Payara Micro. Please consult with the Payara team or patch your Payara Micro version with stock Jersey jars. |
|
@lprimak , please confirm that the issue is gone if you replace Jersey jars in Payara Micro with the stock Jersey 4.0.2 jars (jersey-cdi1x, jersey-cdi1x-servlet, and jersey-server were enough for me to get rid of the issue, maybe even jersey-server is enough). If that's the case, please ask the Payara team for support. I don't think we can accept a fix for something that is not reproducible with the upstream Jersey artifacts. |
|
@OndroMih , As suggested, I ran Payara with stock (un-patched) jersey-cdi1x, jersey-cdi1x-servlet, and jersey-server
I also ran GlassFish 8.0.3. However, because GlassFIsh has many more ClassLoader leaks, I cannot pinpoint which is causing them.
|
|
I ran the debugger with unpatched Jersey 4.0.2
Watch that set is called multiple times, but remove() is not called after each set() Further elaborating, the |
|
Also, this leads to weird subsequent deployment and hanging issues, since |
|
It's possible that GlassFish 8.0.3 has some other leaks but I don't see this specific leak. None of the WebAppClassloader instances in GlassFish after I deploy and undeploy the app is referenced from a ThreadLocal of type ImmediateHk2InjectionManager as in Payara. What I see in GlassFish is several WebAppClassloader instances related to the Admin UI and 1 related to the undeployed app. But that is referenced from a JarFileManager thread, which inherits it via thread context CL from the parent thread. I scanned all thread locals in all threads via a debugger and I didn't find any thread local that would recursively reference a WebAppClassloader instance in GlassFish or Payara with the stock Jersey jar. I tried again replacing Payara's jersey-server.jar in Payara 7.2026.6 with the stock jersey-server 4.0.2 and I stopped seeing the leak. After deployment and undeployment, there's a single WebAppClassloader instance, for the Admin UI, while before I saw what you shared - 2 instances, one of them held by a ImmediateHk2InjectionManager threadLocal. Did you delete osgi-cache after you replaced jersey-server.jar, @lprimak ? Here's what I see in raw Payara 7.2026.6 - 2 instances of WebAppClassloader, one is expected (admin UI) and the other one is held by the thread local:
After I replaced the jersey-server.jar, deleted osgi-cache from the domain directory and repeated the reproducer, I only saw a single instance for the admin console, no other WebAppClassloader instance remaining after undeployment:
In GlassFish 8.0.3, I also don't see WebAppClassloader leaking from the ImmediateHk2InjectionManager thread local, I just see 5 WebAppClassloader instances either leaking via inheritableThreadLocals or held by some other GlassFish components which should probably have released the references:
|
|
@lprimak , please, try again with stock Jersey JARs in Payara and delete osgi-cache. If you still see leaks, please create a script or a Dockerfile which demonstrates what you're doing. I really can reproduce your issue only on the default Payara setup but not with Payara and stock Jersey 4.0.2 or GlassFish 8.0.3 (with the same Jersey 4.0.2). |
|
I not only tried deleting osgi-cache but started with fresh domain. |
|
Also did you actually use the reproducer app? It's not enough just to deploy and undeploy it, but you need to use the deployment. Best way is the debugger like I mentioned above. |
|
Also, you are not going to see this specific leak in GlassFish, because other leaks are hiding this particular leak. |
|
I have created a brain-dead-simple reproducer for you: You will see that there is no |
|
I didn't see this reproducer linked anywhere. I was using https://github.com/AndrewG10i/jakartaFacesTestApp.git which I believe you linked somewhere in one of the issues. Next time, please raise a proper issue on the Jersey or GlassFish project instead of distributing information across multiple issues and PRs. I'm getting lost. Your reproducer also doesn't describe the scenario that leads to the leak. Or do the integration tests trigger the leak? Please, describe what I should do to reproduce it if plain deploy and undeploy isn't enough. Ideally the minimum scenario to reproduce it. |
|
I am trying my best here. |
739f880 to
5cc0461
Compare




…lly block
follow-on to #6049 regression