Bind hooks and reactive flush to their runtime generation - #436
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.
|
From previous PR:
can you open a PR for that? I'm not sure what would be the best way to handle these. Shouldn't sqlite3_interrupt minimize the possibility of timeouts? Leaking the handle and pool seems ok for the lifecycle of the mobile apps, I doubt it would cause a problem long term?
Same as previous point, leaking a few them doesn't seem so bad, unless this causes a crash?
Should be taken care of, the alive flag is indeed replacing that functionality. Right? |
|
@ospfranco I'd like to test with these changes first. I
I'd like us to test with these changes first and only if absolutely necessary, we can add the add the bounded drain PR with the leak |
2 participants
Rebase of #435