margelo / react-native-quick-sqlite

A fast react-native SQLite library built using JSI
https://margelo.io
MIT License
347 stars 22 forks source link

clearState doesn't take care of pending threads #32

Open savv opened 10 months ago

savv commented 10 months ago

I've noticed that the sqlite lib crashes when codepush restarts the app, if there are a pending async queries.

The reason is that it tries to use a runtime that doesn't exist anymore, crashing here.

I had previously tried to mitigate this issue with https://github.com/margelo/react-native-quick-sqlite/pull/16. While correct, it still allows pending executions to complete, causing these crashes.

One way to fix this is to kill ThreadPool's threads from osp::clearState().

@ospfranco any thoughts?

Full stack trace

facebook::jsi::Object::Object (jsi.h:658)
createSequelQueryExecutionResult (JSIHelper.cpp:135)
const::lambda::operator() (bindings.cpp:240)
std::__1::__invoke<T> (type_traits:3918)
std::__1::__invoke_void_return_wrapper<T>::__call<T> (invoke.h:61)
std::__1::__function::__alloc_func<T>::operator() (function.h:178)
std::__1::__function::__func<T>::operator() (function.h:352)
std::__1::__function::__value_func<T>::operator() (function.h:505)
std::__1::function<T>::operator() (function.h:1182)
facebook::react::Instance::JSCallInvoker::scheduleAsync::lambda::operator() (Instance.cpp:295)
std::__1::__invoke<T> (type_traits:3918)
std::__1::__invoke_void_return_wrapper<T>::__call<T> (invoke.h:61)
std::__1::__function::__alloc_func<T>::operator() (function.h:178)
std::__1::__function::__func<T>::operator() (function.h:352)
std::__1::__function::__value_func<T>::operator() (function.h:505)
std::__1::function<T>::operator() (function.h:1182)
facebook::react::NativeToJsBridge::runOnExecutorQueue::lambda::operator() (NativeToJsBridge.cpp:310)
std::__1::__invoke<T> (type_traits:3918)
std::__1::__invoke_void_return_wrapper<T>::__call<T> (invoke.h:61)
std::__1::__function::__alloc_func<T>::operator() (function.h:178)
std::__1::__function::__func<T>::operator() (function.h:352)
std::__1::__function::__value_func<T>::operator() (function.h:505)
std::__1::function<T>::operator() (function.h:1182)
facebook::react::tryAndReturnError (RCTCxxUtils.mm:74)
facebook::react::RCTMessageThread::tryFunc (RCTMessageThread.mm:69)
std::__1::__function::__value_func<T>::operator() (function.h:505)
std::__1::function<T>::operator() (function.h:1182)
facebook::react::RCTMessageThread::runAsync (RCTMessageThread.mm:45)
CoreFoundation
+0x0355c4
<redacted>
CoreFoundation
+0x033ee8
<redacted>
CoreFoundation
+0x031de8
<redacted>
CoreFoundation
+0x031a04
CFRunLoopRunSpecific
+[RCTCxxBridge runRunLoop] (RCTCxxBridge.mm:337)
Foundation
+0x0a805c
<redacted>
libsystem_pthread
+0x00218c
_pthread_start
savv commented 10 months ago

Yet another mitigation could be to sql_interrupt, prior to calling close_v2 from osp::clearState(). If I understood correctly, this should make all pending transactions terminate synchronously, and before the runtime disappears.

ospfranco commented 10 months ago

I took a bit more, but managed to fix the crash when reloading the bundle via react-native-restart. You can give it a try with Codepush and let me know if it works. For now, I will stop spamming margelo since quick-sqlite belongs to them now.

https://github.com/OP-Engineering/op-sqlite/releases/tag/1.0.11

savv commented 10 months ago

Hi Franco, many thanks for addressing this issue! That's a very thorough fix. Just one suggestion.

This comment is no longer accurate in my opinion:

    // In certain cases, this will return SQLITE_OK, mark the database connection as an unusable "zombie",
    // and deallocate the connection later.

It could be replace with sth like: "Calling sqlite3_interrupt will cause all pending (async) execution threads to immediately throw SQLITE_INTERRUPT and sqlite3_close_v2 to return immediately with SQLITE_OK."