Synchronize the lookup-and-define-class logic in blackbird #172
Conversation
| throw e; | ||
| } catch (Throwable e) { | ||
| throw new RuntimeException(e); | ||
| synchronized(CrossLoaderAccess.class) { |
There was a problem hiding this comment.
there may be a narrower thing to synchronize on ? (pkg.getName().intern() perhaps?)
There was a problem hiding this comment.
Was thinking the same thing but intern() itself can be costly enough. So maybe start with safest approach like so.
There was a problem hiding this comment.
Given that this only happens once per class during warmup, I think a coarse lock is fine.
Tiny nit: the other keyword blocks in this class have a space between e.g. catch ( so matching style here would have one more space synchronized (...)
| try { | ||
| return Class.forName(pkg.getName() + "." + CLASS_NAME, true, lookup.lookupClass().getClassLoader()); | ||
| } catch (ClassNotFoundException ign) { } |
There was a problem hiding this comment.
"try again" logic is attempting to do double checked locking -- I think that's ok in this case but could always use more eyes on those attempts. Could also remove the unsynchronized reflection and just do everything in a sync block?
There was a problem hiding this comment.
I think it's nice to avoid unnecessary synchronization in the common path - where the class is already loaded.
It's a little unfortunate that this Class.forName expression is repeated, but factoring it out might not be much clearer.
What about computing the className and fetching classLoader once, stash them in local vars - then at least we don't do the string concatenation twice unnecessarily.
| return (Class<?>) DEFINE_CLASS.invokeExact(lookup, classBytes.toByteArray()); | ||
| } catch (RuntimeException | Error | IOException | ReflectiveOperationException e) { | ||
| throw e; | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
Could be Exception although I suppose does not make much difference as preceding catch block already handles RuntimeException and Error.
There was a problem hiding this comment.
possibly. I didn't want to mess with the existing code much so left it as is other than surrounding with synchronization....
There was a problem hiding this comment.
invokeExact declares throws Throwable and since it's possible to declare your own subtype of Throwable, the RuntimeException | Error doesn't actually cover all possible types. So I think making this change would result in a compiler error. Feel free to play around with it though and see if you can do better!
There was a problem hiding this comment.
Interesting! Did not realize that someone would mark Throwable to be thrown -- don't recall ever seeing that -- but if so, yes, that makes sense.
(also don't remember ever seeing custom Throwable sub-class but now I know :) ).
There was a problem hiding this comment.
This is because invokeExact represents an arbitrary method call, with unknown throws declared. So since you can call a method that might throws Throwable, so too invokeExact must handle this possibility.
This was not needed in classic reflection, since any thrown exception was wrapped in an InvocationTargetException. But, this didn't end up always being good - for example if you wrapped an unexpected Error often the exception handling would get very confusing.
|
Can't find anything wrong but I'll let @stevenschlansker review as he's the author of Blackbird module should know best. |
| throw e; | ||
| } catch (Throwable e) { | ||
| throw new RuntimeException(e); | ||
| synchronized(CrossLoaderAccess.class) { |
There was a problem hiding this comment.
Given that this only happens once per class during warmup, I think a coarse lock is fine.
Tiny nit: the other keyword blocks in this class have a space between e.g. catch ( so matching style here would have one more space synchronized (...)
| try { | ||
| return Class.forName(pkg.getName() + "." + CLASS_NAME, true, lookup.lookupClass().getClassLoader()); | ||
| } catch (ClassNotFoundException ign) { } |
There was a problem hiding this comment.
I think it's nice to avoid unnecessary synchronization in the common path - where the class is already loaded.
It's a little unfortunate that this Class.forName expression is repeated, but factoring it out might not be much clearer.
What about computing the className and fetching classLoader once, stash them in local vars - then at least we don't do the string concatenation twice unnecessarily.
If blackbird attempts to access/define a new class concurrently, it can hit a race condition between attempting to access it and defining it when that fails. If it does fail, enter a synchronized block and try again before defining the access class. Should fix FasterXML#169
josephlbarnett
force-pushed
the
blackbird-synchronized
branch
from
8a2e02b
to
274772f
Compare
|
updated per @stevenschlansker's suggestions. would be good to get this in 2.13 as well once accepted, anything I can do to help with that? |
|
@josephlbarnett For 2.13, it'd be easier if PR was based on 2.13. I can try cherry-picking if that is too much work. My only concern is wrt safety of the fix, but it seems low-risk enough. It'd go in 2.13.3 which should get out quite soon, as well. |
|
#173 is a 2.13 cherry-pick of this commit |
|
Not sure what gives, but JDK 11 fails with one test. |
I see the test failing both before and after this change? not sure what's happened there |
|
@josephlbarnett that makes sense. And yes, I can reproduce the fail locally. I think this was hidden by having JDKs 8 and 14 tested on CI (not 11). But since 14 was dropped by Github it seems (not sure why), changed to 8 and 11 exposing the problem. And I think we'll get plenty more fails with JDK 17 so it's not clear what to do right now. |
Synchronize the lookup-and-define-class logic in blackbird (2.13 cherry pick of #172)
If blackbird attempts to access/define a new class concurrently,
it can hit a race condition between attempting to access it and
defining it when that fails. If it does fail, enter a synchronized
block and try again before defining the access class.
Should fix #169