Fix MyClassLoader idempotency - issue #49 #50
Conversation
|
Sounds good. I have been (and am) swamped with non-OSS work and other things so there may be some delay in processing, but wanted to add a note to say thank you for submitting this. Even if gettng it in may take a while. |
|
@cowtowncoder I just wanted to follow up and ask if there's anything else I can do to help get this change merged in soon. I'm happy to write additional unit tests or comments if you'd like. This issue has bitten me again a couple different times in the past few months, and I'm eager to get to a version with a fix. |
|
Pinging this ticket. I've hit this error several times as well. |
| * @param className The name of the to-be-loaded class | ||
| * @throws IllegalStateException if there is no parent ClassLoader or the lock object could not be retrieved | ||
| */ | ||
| private Object getParentClassLoadingLock(ClassLoader parentClassLoader, String className) |
There was a problem hiding this comment.
Is there particular reason it must be something internal to JDK, to lock on? Access to that object is potentially quite expensive, and with later JDK versions likely very fragile.
There was a problem hiding this comment.
I think it needs to use the parent ClassLoader's internal lock for complete correctness. ClassLoader#loadClass acquires the lock, then (potentially) calls ClassLoader#findClass, which may ultimately call ClassLoader#defineClass. #defineClass will fail if it is called twice for the same class name (which is guarded against by the synchronization in #loadClass). Acquiring the same lock used by ClassLoader#loadClass guarantees that no other mechanism (outside of Afterburner) could load a class with the name we are using between the time we check to see whether it is defined and attempt to define it.
In our case, though, if we assume that MyClassLoader will be the only class loader implementation than attempts to load/define a class with a specific name (a name which always includes the 'Creator4JacksonDeserializer' suffix and the bytecode hash, so this seems like a safe assumption), then we should be safe in relaxing this requirement and just synchronizing on our own static lock.
There was a problem hiding this comment.
I'd feel better about our own lock unless we can think of specific case where someone or something might try to load class with same name concurrently; at least outside of Afterburner.
If the concern is for concurrent Afterburner instances from same JVM, probably would be ok to use static singleton lock (... yeah, not a big fan of those things, who is, but here it seems inevitable...). And I guess that is a valid concern, so even though ideally one tries to avoid multiple instances of ObjectMapper -- and esp. so with Afterburner generating classes on the fly! -- there are some legit cases when 2 or 3 mappers are not for different configurations.
So, if this could be done that is great.
Also: just so I won't forget to follow up, I added this on:
https://github.com/FasterXML/jackson-future-ideas/wiki/Jackson-Work-in-Progress
as an urgent issue.
|
Ok so as per description of the problem this definitely seems worth fixing. One question I have has to do with locking aspects; I am curious as to whether lock object needs to be an internal lock by JDK, or if something else could plausibly be used. I realize that it may be difficult to build locking at suitable scope (I think it'd need to be static synchronized map, keyed with ClassLoader + unique-class-name-hash-code combo?), but if that would work I'd prefer route where implementation details of JDK's class loading would not need to be relied on. I added this issue/PR on my WIP list on: https://github.com/FasterXML/jackson-future-ideas/wiki/Jackson-Work-in-Progress |
|
Thanks for your feedback. I'll add a commit that switches to using locking internal to Just wanted to flag that |
| String key = String.format("%s:%d:%s", | ||
| parentClassLoader.getClass().getCanonicalName(), | ||
| System.identityHashCode(parentClassLoader), | ||
| className); |
There was a problem hiding this comment.
nit: Might want to avoid the relatively expensive String.format when generating lock names and just concatenate instead.
There was a problem hiding this comment.
Oh. Yes. String.format is HORRENDOUSLY slow, for what it does -- fine for exception logging (f.ex), but not for even generating not-in-busy-loop ids. Alas.
| System.identityHashCode(parentClassLoader), | ||
| className); | ||
| Object newLock = new Object(); | ||
| Object lock = parentParallelLockMap.putIfAbsent(key, newLock); |
There was a problem hiding this comment.
Do we need a corresponding parentParallelLockMap.remove(key) at the end of the critical section before releasing this lock to avoid this map growing unbounded?
There was a problem hiding this comment.
I don't think it will work to simply expire entries from the map when the lock is released. Consider this sequence of operations contending for a lock for the same class name/loader pair (i.e. with the same map key: 'key'), beginning with an empty map:
[Thread A]: requests lock object for 'key'; map contains no entry for 'key' so Object-1 is created and added to map, then returned.
[Thread A]: attempts to acquire lock for Object-1; succeeds immediately.
[Thread B]: requests lock object for 'key'; receives Object-1 from map.
[Thread B]: attempts to acquire lock for Object-1; blocks.
[Thread A]: completes its work in the synchronized region and then (in either order) releases the lock on Object-1 and removes the 'key' -> Object-1 mapping from the map.
[Thread B]: immediately acquires lock for Object-1 and unblocks.
[Thread C]: requests lock object for 'key'; map contains no entry for 'key' so Object-2 is created and added to map, then returned.
[Thread C]: attempts to acquire lock for Object-2; succeeds immediately.
Now, Thread B and Thread C are both in the synchronized region together for what should be the same lock.
A Guava (or Caffeine) Cache with weak values would do the trick, but I doubt it's worth adding that dependency. For what it's worth, though, this is the same implementation as used by ClassLoader itself. Its lock map also grows without bound (but, since it only ever gains one entry per loaded class it can't really grow that big)
There was a problem hiding this comment.
Good call @jbagdis , having a corresponding remove does allow for the race condition you mentioned.
|
Ok -- apologies for delay here, but I will try to re-read this. One last (I hope!) thing: @jbagdis unless I have asked for and gotten CLA (if so, apologies -- we have quite a few and sometimes I miss one we have on file), we would need it from (or corporate-CLA alternative) |
|
Ok, got CLA, merged! One problem is merging to |
|
Thanks @cowtowncoder for getting this merged! |
|
Any idea what release this change will be a part of and when that will be made available? |
|
@danrwhitcomb Yes, as "milestone" indicates, will be part of 2.9.9 (and 2.10.0). When those get released I don't know at this point -- we are far enough with 2.9.x releases that it may take a while. |
|
@cowtowncoder are there plans to get 2.9.9 out soon? Would love to get the fix! |
|
Unfortunately I am rather busy at work so haven't had much time to spend on Jackson lately. |
|
Got it thanks for the update. |
…mpotency Fix MyClassLoader idempotency - issue FasterXML#49
Adds to failing unit tests that indicate the behavior described in issue #49, and a proposed solution that addresses the problem and resolves the tests.