Bind async work to its JS runtime generation - #435
Closed
babzcraig wants to merge 8 commits into
Closed
Conversation
During a bridgeless reload two JS runtime generations overlap: the outgoing one is invalidated while the incoming one is already installing. The process globals invoker and invalidated cannot describe that, so a database created by the outgoing generation would post its hook and reactive-query callbacks into the runtime that replaced it, and then call asFunction() on a jsi::Value owned by the dead one. Each generation now gets its own liveness flag, and DBHostObject binds both the invoker and that flag at construction, on the JS thread. The members shadow the globals inside member functions, so every invokeAsync site is fixed without touching each call site.
Same problem as the hooks, on the thread pool. A task read opsqlite::invoker when it completed rather than when it was queued, so a query started by the outgoing runtime could resolve into the incoming one and call asFunction() on a jsi::Value that belongs to the dead runtime. The invoker and the liveness flag are now captured on the JS thread while the promise is constructed, and the early return checks this generation rather than the process-global invalidated, which is set by whichever generation tore down last. Note the early return still drops the last references on the pool thread, so ~jsi::Value runs there. That is pre-existing, and the sqlite3_interrupt change that follows makes it very hard to reach, but it is not airtight without either leaking the values or handing them to the JS thread.
invalidate() waits on the thread pool but, unlike close() and delete(), never asks SQLite to stop. A query inside sqlite3_step therefore runs to completion while the JS runtime is being torn down. React Native gives module invalidation ten seconds before it destroys the runtime regardless, so an uninterrupted drain can lose that race and leave a worker touching state the runtime owned. This is the same three lines close() already runs, moved to the one teardown path that lacked them.
open() runs on the JS thread and invalidate() runs on the TurboModule queue. During a bridgeless reload those belong to different generations and overlap, because the new instance is constructed before the old one finishes invalidating, so both can reach the dbs vector at once. Iterating it while another thread emplaces can walk a reallocated buffer. invalidate() now swaps the vector out under the lock and works on its own copy, which also keeps the lock off the invalidate() calls themselves.
waitFinished() and doWork() wait on the same condition variable, so notify_one() can wake a worker when the waiter needed the wakeup, or the other way round, and leave the other one asleep until the next event. With a single pool thread that is easy to hit at teardown, where waitFinished() is the only waiter that matters. done is read in the doWork() loop condition outside the mutex while the destructor writes it, which is a data race; make it atomic.
If the last shared_ptr to a ThreadPool is released on one of its own workers, the destructor tries to join the running thread and std::thread::join throws "Resource deadlock avoided", which aborts. Dropping the pool capture from the promisify task removes the known way to get there, so this is a backstop for any future owner that ends up released on a worker.
Contributor
|
Going to close this as I cleaned up a bit the code in #436 (and it's stack). I will still credit you :) Moving some of the comments there |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #434. This is the per-generation liveness work you asked for, plus the smaller teardown races we found alongside it.
Your two commits from #434 are cherry-picked underneath, unchanged and still authored by you, so this branch applies to
mainon its own. If you would rather merge #434 first, do that and I will drop them and rebase.The problem
React Native's bridgeless reload does not tear one runtime down and then build the next.
RCTHostconstructs the newRCTInstancewhile the old one is still invalidating, so two generations overlap. The process globalsopsqlite::invokerandopsqlite::invalidatedcannot describe that: whichever generation wrote last wins. A database created by the outgoing runtime would read the global invoker when its work completed, post into the runtime that replaced it, and callasFunction()on ajsi::Valueowned by the dead one.The commits
opsqlite::generation_alive, ashared_ptr<atomic<bool>>replaced byinstall()and cleared byinvalidate().DBHostObjectcaptures the invoker and that flag at construction on the JS thread. They shadow the globals inside member functions, so everyinvokeAsyncsite in the class is fixed without editing each one.invalidate()waited on the pool but never calledsqlite3_interrupt, unlikeclose()anddelete(). React Native abandons module invalidation after ten seconds and destroys the runtime anyway, so an uninterrupted drain can lose that race.open()runs on the JS thread andinvalidate()on the TurboModule queue, and during a reload those overlap, so both can reachdbsat once.invalidate()now swaps the vector out under the lock and works on its own copy.waitFinished()anddoWork()share one condition variable, sonotify_one()can deliver the wakeup to the wrong waiter.doneis read outside the mutex while the destructor writes it.join()throwing "Resource deadlock avoided" if a future owner releases the pool on one of its own workers. Dropping the pool capture in Patches around async work #434 removes the known route there.Deliberately left out
Our patch also bounds the drain (
waitFinished(timeout)) and, on timeout, leaks the pool and the sqlite handle rather than freeing them underneath a live worker. That is a policy call about what to do when a query will not stop, and it did not belong inside a crash fix, so it is not here. Happy to open it separately if you want it.Known limit
The early return in
promisifystill drops the last references on the pool thread, so~jsi::Valueruns there rather than on the JS thread. That behaviour is pre-existing, and thesqlite3_interruptchange makes it very hard to reach, but it is not airtight without either leaking the values or handing them to the JS thread.Also worth noting: after this change,
opsqlite::invalidatedis written but never read, sincepromisifywas its only reader.