Fix for context classloader issue #58
Conversation
…tup of `HazelcastSessionManager`
| @@ -1,3 +1,7 @@ | |||
| /* | |||
| * Copyright (c) 2008-2016, Hazelcast, Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
Checkstyle's class header is set as 2016, I needed to keep it as it is to make it pass from the checks. I will send another PR for updating all headers.
| instance = Hazelcast.getOrCreateHazelcastInstance(P2PLifecycleListener.getConfig()); | ||
| } | ||
| instance = HazelcastInstanceFactory. | ||
| getHazelcastInstance(getContainer().getLoader().getClassLoader(), isClientOnly(), getHazelcastInstanceName()); |
There was a problem hiding this comment.
is this WebAppClassloader that is returned?
There was a problem hiding this comment.
I mean by getContainer().getLoader().getClassLoader()
There was a problem hiding this comment.
Yes, I double checked.
| */ | ||
| Config config = P2PLifecycleListener.getConfig(); | ||
| config.setClassLoader(classLoader); | ||
| instance = Hazelcast.getOrCreateHazelcastInstance(config); |
There was a problem hiding this comment.
if instanceName is null then is there any advantage using Hazelcast.getOrCreateHazelcastInstance? Can't we just start an instance?
There was a problem hiding this comment.
There is still possibility for the Hazelcast instance to be up and running even that the instanceName is null. If the instance name is set in the Hazelcast config, then the instanceName variable will be null but there will be still a running Hazelcast instance.
There was a problem hiding this comment.
I see this is too confusing. We really need to check why we have a separate hazelcastInstanceName apart from hazelcast configuration... READMe is not clear too.
There was a problem hiding this comment.
It is because we have a SessionManager.DEFAULT_INSTANCE_NAME to bring back the existing instance, if the user doesn't set up a instance name in the config. Yes, it is a bit confusing, I can try to clarify in the readme.
| /* | ||
| Note that Hazelcast instance can only set a classloader during the initialization. If the context classloader | ||
| changes after the initialization of the Hazelcast instance, it cannot be changed for the Hazelcast instance. | ||
| */ |
There was a problem hiding this comment.
so does that mean that if there is hazelcast instance in the VM and classloader is not somehow set to WebAppClassloader then there will be an issue.
We might need to clarify this in the readme.... Also isn't this comment for instanceName != null branch?
There was a problem hiding this comment.
Yes, that means if there is Hazelcast instance in the VM and classloader is not set to WebAppClassloader then there will be an issue.
If instanceName != null, Hazelcast instance should be already up and running, thus you cannot change the classloader. So; yes, it is also the same case for that branch. But I didn't put the comment there since we don't even try to set the classloader over there.
Fixes #55.
This fix moves the initialization of the Hazelcast member instance on P2P mode to the startup of
HazelcastSessionManagerin order to access the context classloader. Please note that there might be still an issue if the context classloader changes dynamically, since Hazelcast doesn't allow to update the classloader after the initialization of the instance.