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

Explicitly pass ClassLoader of XmlFactory when creating Stax input/output factory, instead of context ClassLoader #483

Closed
cowtowncoder opened this issue Jul 4, 2021 · 6 comments
Milestone

Comments

@cowtowncoder
Copy link
Member

(for background see PR #480)

When creating XMLInputFactory / XMLOutputFactory instances -- in cases where user does not provide explicit instances -- current code uses no-argument static factory methods, which ultimately end up using context ClassLoader to locate implementation via SPI. This can lead to sub-optimal selection process, and it seems better to instead explicit pass the ClassLoader to use, and specifically pass (by default) ClassLoader that was used to load XML module class XmlFactory.

Before making the change it would be great to have some supporting documentation/articles explaining common reason for doing this, and/or something explaining potential trade-offs.

Another thing to consider would be whether to allow configuring this aspect (maybe simple on/off to toggle between) or not: since user may simply opt to instantiate and pass factories on its own, configurability may not make much sense (if caller has to do something they might as well just pass instances to avoid all fragility of SPI approach).

@cowtowncoder
Copy link
Member Author

cowtowncoder commented Jul 5, 2021

Ok, as per highly rated answers on:

https://stackoverflow.com/questions/1771679/difference-between-threads-context-class-loader-and-normal-classloader

it does appear there is somewhat of a consensus on:

  1. Never rely on context classloader,
  2. Usually class loading another class should use ClassLoader that loaded it, and
  3. If class that does loading does it on behalf of someone else, it should take in ClassLoader

all of which is to say that yeah #480 makes sense. I think what I'll do for 2.13 is along these lines so that

  1. Default behavior will be to use ClassLoader of XmlInputFactory (or whichever local class calls Stax factory method)
  2. Allow passing ClassLoader through XmlFactoryBuilder, to let caller change it as it sees fit.

While (2) is sort of optional in the sense that caller could as well as handle instantiation it is not a lot of code to add and might make it easier for users to realize potential issues and (in case they hit issues first) find solution(s).
Javadocs for the config method should state that there is the alternative way of simply passing factories directly.

@timja @jglick WDYT?

@timja
Copy link

timja commented Jul 5, 2021

Sounds perfect,

I think 2 is worth doing

@cowtowncoder
Copy link
Member Author

Ok I'll add in my todo list & try to get it done for 2.13.0-rc1!

@cowtowncoder
Copy link
Member Author

Implemented -- hope I didn't botch anything; bit tricky to test (as unit tests explicitly define xml input/output factory).

@basil
Copy link

basil commented Jul 6, 2021

Verified the final fix in a realistic Jenkins test environment. Many thanks for your work on this!

@cowtowncoder
Copy link
Member Author

@basil np, glad to be able to improve handling based on good analysis, suggestions.

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

No branches or pull requests

3 participants