rodionmoiseev / c10n

A Java library, focused on making internationalisation more modular, easier to evolve and maintain, robust-to-change and IDE-friendly without excess of external tools.
Apache License 2.0
67 stars 10 forks source link

Multiple classloaders and thread safety #37

Closed mredaelli closed 8 years ago

mredaelli commented 8 years ago

Hello!

I would love to use c10n in a project where I have several bundles in an OSGi environment.

I have seen the ticket in which you show how to have different configurations of c10n for different libraries, but in this case it is the classloader for c10n has to be set in every package to the classloader of that package. I could use C10N.setProxyClassloader, but if between that call and the creation of the package's instance, another package in another thread is initializing its own instance of c10n, I don't think I'm guaranteed to get what I want.

Is this possible?

rodionmoiseev commented 8 years ago

Hi!

Sorry, I am not quite following what exactly is the issue you are trying to solve. I believe, there should be no problem just using c10n in one or more OSGi bundles, without any special configuration, as each bundle will be loaded with its own classloader, therefore loading multiple separate instances of C10N class and other classes. So provided you don't attempt to explicitly access c10n classes loaded in the classloader of another bundle (which is quite tricky and probably not so useful) there should be no concurrency issues.

Not sure exactly about how OSGi class loading works though. Does it attempt to reuse the class from a classloader of another unrelated bundle if the same version of a dependency is used, or something like that??

mredaelli commented 8 years ago

To be honest I never really investigated the issue of statics and OSGi myself, but I found this, from which I quote:

Bundles are used by other bundles. So if a bundle is used by another bundle instead of duplicating the jar file, then there is only a single instance of the static variables. And the class loaders form hierarchies.

In my case, there is a single c10n bundle, used by all other instances, so I think the thread problem is actually there.

rodionmoiseev commented 8 years ago

OK, I see the issue now. Yes, from that description it seems that classes declared in the c10n bundle will be loaded by the same classloader and shared in the different depending bundles. So if you use any static methods inside C10N class there may be issues, as all of your bundles will share the c10n configuration as well as all the special interface proxy classes.

The concurrency issue is present, however, actually it would be the same issue if you used c10n in a threaded environment in a normal single-classloader application, because there is no synchronization. If this was the only issue, you could just wrap the access to all static C10N methods with a lock on C10N.class instance.

In your case, I think it would be a lot safer to simply avoid using any statics in C10N and create your own c10n factory instances per bundle. You can check the source of C10N.java to see how that is done.

You will have to be careful with the proxy classloader however, as it is hardcoded to C10N.class classloader. I will give you a sample on how to approach your problem, including ther setProxyClassloader hack that will be needed to make user all bundles use their own classloader for proxy creation:

//somewhere in the bootstrapping code of the individual bundle
//you will need to configure you c10n message factory instance

C10NCoreModule coreModule = new C10NCoreModule();
//Create configured c10n module with all of your settings as you would
//normally do with C10N.configure(...);
ConfiguredC10NModule configuredModule = coreModule.resolve(new C10NConfigBase(){
    @Override
    public void configure() {
      //do your config here as usual
      install(new DefaultC10NAnnotations());
    }
  });
//Create the factory based on the configured module
//This needs to be synchronized to make sure C10NMsgFactory instance is
// created with the correct proxy classloader without the risk of being overwritten
// by another thread (bundle) in the interim
C10NMsgFactory c10nFactory; 
synchronized(C10N.class){
  C10N.setProxyClassloader( SomeClassDeclaredInThisBundle.class.getClassLoader() );
  c10nFactory = coreModule.defaultC10NMsgFactory(configuredModule);
}

//Now you can safely use the c10nFactory from your bundle, as it will not share any
//state with other instances for c10nFactory in other bundles.

MyMessages msg = c10nFactory.get(MyMessages.class);

You can save the c10nFactory into some static variable for easy access from all of your code, provided it's a static variable declared in a class in this bundle. Or so I understood.

Hope that helps!

mredaelli commented 8 years ago

Sorry for the late answer!

Yes, that approach would work, thanks. I don't know if you hardcoded the classloader like that for a particular reason, but wouldn't it be better to make it a field in the factory, and pass it as part of the configuration?

It would avoid that synchronized, which in some cases can be problematic. Take my example. I have to deploy several bundles, all of which contain several classes that inherit from an abstract class B, deployed in its own bundle, which provides basic functionality. Part of that functionality is the setting up of c10n. Upon construction of the derived classes I'd like to create their own c10n instance. I cannot use a common instance in the base class B, because it's in its own bundle and has no access to the classes in the other bundles. But this means that the synchronized code gets potentially called a lot - which I'd like to avoid.

rodionmoiseev commented 8 years ago

Good to know it worked for you.

Originally, I did not take into consideration scenario where you'd need to configure multiple instances in different threads. But since with OSGi there seems to be a need for this. I will think about refactoring it a little bit to avoid all the synchronization complexity.

Thanks for the feedback!

rodionmoiseev commented 8 years ago

Please check out the latest 1.3-SNAPSHOT version of c10n-core. It should be now possible to achieve the same as the workaround I gave above with with much simpler code (and no synchronized):

//Create the factory with your bundle classloader. The call below does not
//set any mutable static variable, and is therefore thread safe.
C10NMsgFactory msg = C10N.createMsgFactory(new C10NConfigBase() {
            @Override
            protected void configure() {
                //do your config here as usual
                install(new DefaultC10NAnnotations());

                //set-up your bundle classloader
                setProxyClassLoader(SomeClassDeclaredInThisBundle.class.getClassLoader());
            }
        });

//Now you can safely use the c10nFactory from your bundle, as it will not share any
//state with other instances for c10nFactory in other bundles.
MyMessages msg = c10nFactory.get(MyMessages.class);
mredaelli commented 8 years ago

Perfect! Just was I was hoping for :)

Thanks