Fixed deadlock when sealing objects with a LazyCtor #1476
Conversation
The reason is that ScriptableObject::sealObject will initialize eagerly any LazyLoadedCtor that are stored in the slot map - it needs to that because some of those will actually mutate the object (such as NativeJavaTopPackage), and that cannot work if the object is already sealed. However, it will also acquire a read lock on the slot map before iterating on it. But the mutations will try to acquire a write lock, causing a deadlock with just one thread. This change fixes it by not holding the read lock while mutating, and also avoids marking the object as sealed until it is certainly correct to do so.
|
Cool — I’m glad that someone got to dig in to this capability of Rhino.
I’m curious — what are you using the thread-safe features in Rhino for? I
have considered in the past that we should remove most synchronization from
Rhino because I can’t prove that it’s truly thread-safe, at least when
sharing objects and Contexts across threads.
Running lots of instances of Rhino Contexts in parallel should always be
thread-safe, but when the same script, or the same set of objects, is used
by multiple threads I am not convinced that we really have (or ever had)
synchronization support for anything else, and most other JS engines I know
of don’t even try.
…On Fri, May 17, 2024 at 5:10 AM Andrea Bergia ***@***.***> wrote:
The reason is that ScriptableObject::sealObject will initialize eagerly
any LazyLoadedCtor that are stored in the slot map - it needs to that
because some of those will actually mutate the object (such as
NativeJavaTopPackage), and that cannot work if the object is already
sealed. However, it will also acquire a read lock on the slot map before
iterating on it. But the mutations will try to acquire a write lock,
causing a deadlock with just one thread.
This change fixes it by not holding the read lock while mutating, and also
avoids marking the object as sealed until it is certainly correct to do so.
------------------------------
You can view, comment on, or merge this pull request online at:
#1476
Commit Summary
- 94246fb
<94246fb>
Fixed deadlock when sealing objects with a LazyCtor
File Changes
(2 files <https://github.com/mozilla/rhino/pull/1476/files>)
- *M* src/org/mozilla/javascript/ScriptableObject.java
<https://github.com/mozilla/rhino/pull/1476/files#diff-8714999b8456df8a1e83229d16130d3d5de6284621681c4091e67fe5fc99de34>
(39)
- *A*
testsrc/org/mozilla/javascript/ThreadSafeScriptableObjectTest.java
<https://github.com/mozilla/rhino/pull/1476/files#diff-3b6372f3a8542bbe5b21f41c762bef3a4429c4480154ee6f188fe881c6cd2593>
(30)
Patch Links:
- https://github.com/mozilla/rhino/pull/1476.patch
- https://github.com/mozilla/rhino/pull/1476.diff
—
Reply to this email directly, view it on GitHub
<#1476>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I242LTVGYS2OOMGDGHTZCXXTJAVCNFSM6AAAAABH4ATOB6VHI2DSMVQWIX3LMV43ASLTON2WKOZSGMYDENJXHE4DMMA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
The reason is that we are in process of upgrading our version of rhino, and we caught up 1.7.14 recently. In the (very) old version we used to use, objects had implicitly thread-safe, and so there are some code paths where they are shared after being created (like stored in queues, memory caches, that sort of things). |
There was a problem hiding this comment.
Thanks -- I can see that this resolves the deadlock, but I'm concerned that it's still not thread-safe. In particular, in the new loop, we're modifying the values of the slot map without a lock. At the very least, I think that we should get a read lock in the finally clause in line 2000. Even then, I'm not sure that fixes everything.
I could also argue that since this is only for the specific case of a "lazy constructor" that happens only once per run, that this might all be academic, but it's another reason why I have been very wary to tell people that the "thread-safety" in Rhino is real.
| for (Slot slot : toInitialize) { | ||
| // Need to check the type again, because initializing one slot _could_ have | ||
| // initialized another one | ||
| Object value = slot.value; |
There was a problem hiding this comment.
Here, we're reading the "value" field of the slot without a lock being held.
| try { | ||
| initializer.init(); | ||
| } finally { | ||
| slot.value = initializer.getValue(); |
There was a problem hiding this comment.
Here, we're setting the value of the slot without the lock being held.
There was a problem hiding this comment.
This would be a bit safer if it were instead:
long stamp = slotMap.writeLock()
slot.value = initializer.getValue();
slotMap.unlockWrite(stamp)
There was a problem hiding this comment.
So this doesn't seem consistent with the way locks are used elsewhere in the code. Specifically the locks on slot maps are only for reading and mutation of the slot map, not the values of the slots. The reason we hit the deadlock isn't that we're setting the value for the slot being initialised, but rather because the lazy constructors for Java packages create additional slots.
Setting the value on a normal slot is thread safe, it's just setting a field, though I guess we could add var handle support if concurrent operations are needed with specified ordering, but since setting the value doesn't change the slot map no lock is required to ensure thread safety.
Lazy constructors themselves appear to be thread safe since they are synchronised, but there is an amusing possible ordering problem because a lazy constructor could set its own slot value to something, seal the scope, and then return a completely different value which would then be set on the slot. That's technically fine because the object is sealed rather than frozen, but Rhino seems to already be wrong in its treatment of sealed and the mutation of properties in several places.
There was a problem hiding this comment.
Yeah, you're right and I misremembered what we were trying to do here. The lock protects the map and not the value, which means that this change doesn't change the purpose of what the lock is trying to do.
I still maintain that Rhino was never really designed properly for the case of multiple threads executing in the context of the same script in parallel, and if it did we'd have to make up lots of new rules because the standard doesn't contemplate that use case. But for projects that have done this in the past, this particular support at least helps a bit.
Yep, we know that it is not technically not thread safe... but if you have a thread that is mutating the slots, and a thread that is trying to seal that object, you already have a race condition. Some of the mutations might go through, some might not, but you shouldn't even be trying. :-) |
|
Thanks -- I understand what we're fixing now! |
The reason is that ScriptableObject::sealObject will initialize eagerly any LazyLoadedCtor that are stored in the slot map - it needs to that because some of those will actually mutate the object (such as NativeJavaTopPackage), and that cannot work if the object is already sealed. However, it will also acquire a read lock on the slot map before iterating on it. But the mutations will try to acquire a write lock, causing a deadlock with just one thread.
This change fixes it by not holding the read lock while mutating, and also avoids marking the object as sealed until it is certainly correct to do so.