-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Create friendlier API for swapping current thread context classloader #3090
Comments
it is really only about the context classloader since some frameworks just "assume" that this is where the application is running. OSGi has the same problem: http://njbartlett.name/2010/08/30/osgi-readiness-loading-classes.html I would be just plain
where:
|
actually in case |
The name setup_context_classloader says it will setup the classloader but does nothing to imply with what it will set up with or to what it will set it. So I don't think the name really says what is going on. I do not love my name either though so I am just pointing out what I think is a naming issue. The other thought I had was whether we should add '!' onto the end to imply side-effect or definite change in behavior from that call onward. |
how about just keeping the name the same "as is" : JRuby.set_context_class_loader +1 for allowing to pass a custom class-loader : def JRuby.set_context_class_loader(loader = JRuby.runtime.jruby_class_loader)
java.lang.Thread.currentThread.setContextClassLoader loader
end naming should be consistent its |
@kares looking back at the original snippet I guess I agree with you that set_context_class_loader could match context_class_loader (which of course is currently named context_classloader atm). So unless @mkristian or anyone else has input here we should add a second method jruby_class_loader and then add the set method as you have above. I am not sure if we should put this into the FAQ. We have had multiple issues opened about this change. |
+1 about the naming and the implementation of @kares regarding the FAQ it would be good to encourage people to first find a way to tell their java libarary which classloader to use before using this workaround. |
great, go for it ... someone :) regarding FAQ: people running into these should understand a bit of class-loaders (although for sure in one case it seemed not the case) since they import libraries which assume to be able to access the class-es across the class-path. for me no issue seemed "fatal" (for now) and actually some might benefit from changing how their class-path is set, would maybe mention some of it : loading jars in 9K changed in way that the (thread) context class-loader is no longer changed when using |
No test written....but it is there. |
This works but since we have had 2-3 issues opened and we need to give a fairly complex snippet of code (above snippet) I think we should try and add something simpler to use:
where:
I spent about 10 seconds coming up with this so it is mostly just an attempt at getting people to think how we can make this simpler. Also do we need to care about more than context classloaders?
The text was updated successfully, but these errors were encountered: