Use the default XMLInputFactory instance #127
Conversation
|
If you need help with breaking your tests, just ask :). |
|
Yeah I’m perfectly aware this doesn’t work in the tests. Thus why I asked for some insights. |
|
I guess that it's due to not having any such factory with that name available outside of Quarkus (or another environment which specifically provides it). Is the important part of the change the factory ID, or the class loader? If it's the class loader, could you see if using a factory ID of |
|
Tests still fail when changing to |
|
I guess this is due to the lack of a fallback class in this case. We probably need a utility method which tries |
|
This error is thrown when the factory is created: |
|
Yeah there is no nice |
|
As a final fallback, we should probably be calling |
|
Using this makes the tests pass: private static XMLInputFactory newXMLInputFactory(ClassLoader classLoader) {
try {
return XMLInputFactory.newFactory("plexus-components-factory", classLoader);
} catch (FactoryConfigurationError e) {
return XMLInputFactory.newDefaultFactory();
}
}But I am not sure if the first statement will ever work 🫤 |
|
I'd go with making a That method should first try a |
|
Do we ever set the |
|
I'm inferring a lot from @gsmet's original change, but I'm guessing that for whatever reason, the Plexus libraries include their own XML implementation that in turn is useful for us to use for some reason. If using |
|
FWIW, I cannot reproduce the issue anymore with or without this patch (it was reproducible always when I worked on this, not sure what's going on...). |
|
Do we know why exactly this change was necessary in the first place? |
|
I'd be -1 on merging it until we clarify it |
|
Yeah sorry, I should have clarified from the get go but I was building experiments on top of experiments and it was hard to get a clear status. Actually, I'm not reproducing my original issue anymore but with my safeguards in place, I can reproduce the fact that SmallRye BeanBag tries to use a closed CL to load some resources/classes. The original issue I want to solve is that I want to be able to clean the resources of the I started a patch cleaning the resources, but then I had very weird side effects which pointed out the fact that closed CLs were still in use in parts of Quarkus. I added some code to detect these cases and got several occurrences of SmallRye BeanBag trying to access a closed CL. For instance: With this patch, I don't have the issue anymore as we use the CL passed to BeanBag - which is a low level JDK CL -, instead of the You can reproduce the issue with the branch I pointed above:
Now:
Now I'm not entirely sure what's specific with the |
|
It looks like the previous impl would use |
There are 2 different problems with the OpenTelemetry tests:
Also, OTel brings a lot of jars and classes. |
| XMLStreamReader xr = XMLInputFactory.newFactory(XMLInputFactory.class.getName(), classLoader) | ||
| .createXMLStreamReader(br); |
There was a problem hiding this comment.
Actually, I'm not reproducing my original issue anymore but with my safeguards in place, I can reproduce the fact that SmallRye BeanBag tries to use a closed CL to load some resources/classes.
If it's all about using a closed CL due to service loader shenanigans, then I'd recommend this simpler solution:
| XMLStreamReader xr = XMLInputFactory.newFactory(XMLInputFactory.class.getName(), classLoader) | |
| .createXMLStreamReader(br); | |
| XMLStreamReader xr = XMLInputFactory.newDefaultFactory() | |
| .createXMLStreamReader(br); |
This avoids class loading completely, and just directly instantiates the factory from the JDK.
There was a problem hiding this comment.
Let me give it a try.
There was a problem hiding this comment.
@dmlloyd I adjusted the PR to follow your suggestion and the tests pass and the issue is solved in Quarkus too so I would say let's go for it.
@aloubyansky I let you have a look too.
If things are fine for both of you, I would appreciate a quick release so I can iterate on my side. Thanks!
gsmet
changed the title
|
@dmlloyd thanks for the pointer and the release, I included the upgrade in my patch. |
This is part of my work to reduce the consequences of CL leaks in Quarkus.
I think this is the right thing to do... but while it fixes some issues I have in Quarkus... it makes the tests here fail loudly:
I tried to pass a few different CLs to the tests but it didn't fix anything.
@dmlloyd any chance you could have a look, I know you love class loaders :)