react-native-quick-sqlite

react-native-quick-sqlite copied to clipboard

To reproduce:

1. npx react-native init BlobRepro --template react-native-template-typescript && cd BlobRepro
2. yarn add react-native-quick-sqlite && npx pod-install
3. Add a function to write maby blobs, e.g.:

  const dbTest = () => {
    const db = open({name: 'test_data.sqlite'});
    const res = db.execute('CREATE TABLE TEST (data blob)')
    // console.log({res})

    const arr = [...Array(100).keys()];
    console.log({a: arr})
    arr.forEach(i => {
      const res2 = db.execute('INSERT INTO TEST (data) VALUES (?)', [new ArrayBuffer(8)])
      console.log({res2})
    });

    db.close()
  }

4. yarn start --reset-cache , yarn ios
5. Run the test code, e.g. via <Button title='dbTest' onPress={dbTest} />
6. Observe app crashes:
7. Observe only some of the data is present in the sqlite database:

Hmm I did not do the blob integration myself, it was added on https://github.com/ospfranco/react-native-quick-sqlite/pull/12.

@andymatuschak any idea what could be going on?

You seem to have run into some Hermes memory allocation issue/race condition. Don't think there is anything wrong the the code in the library. The stack trace doesn't show any of the library code but just internal Hermes functions.

The implementation does mention that ArrayBuffers are shared around the memory... so maybe something funky is going on there.

Hey, implementation of blobs in React Native is platform-specific. Basically, each blob has its own UUID and is stored in a hashmap in Java/Objective-C. From what I remember, there may be a slight delay between creating blob in JSI vs. adding an entry in the hashmap on the native side. Have you tried flushing the native queue before accessing blobs?

How would you do that?

One way would be to call some blocking synchronous method of native module. This could prove my hypothesis but this is rather unacceptable as it stops JS runtime until the native queue is flushed. Perhaps blob implementation in React Native should be rewritten using JSI so the calls are synchronous. This sounds like an ideal use case for TurboModules from the New Architecture.

I'm not sure I'm following, but current implementation is already using synchronous JSI calls.

Oh sorry, I misread that, you meant the RN blob implementation... I guess you mean the ArrayBuffer, which would fall again under hermes. Does this happen under JSC?

@ospfranco I am also experiencing crashes under JSC

BlobRepro(15342,0x16dd7f000) malloc: *** error for object 0x11e7f00e0: pointer being freed was not allocated

Error originates from facebook::jsc::JSCRuntime::createFunctionFromHostFunction

Could this be relevant for fixing the issue in hermes? https://github.com/facebook/hermes/commit/e6d887ae96bef5c71032f11ed1a9fb9fecec7b46

No, I don't think so. That issue would (I believe) let us remove this extra copy I noted here on reads, but this issue pertains to writes. Furthermore, that API's absence shouldn't affect correctness: we're making a full copy, so anything we do with that memory subsequently should be independent of whatever Hermes is up to.

My first impression here is that JSI is violating its API contract here: our JSI function is being invoked with an array buffer argument, and that buffer is (apparently) being freed concurrently before our function returns control. It's possible that I've misunderstood the contract, but if that's the case I don't see where, nor do I see how it might be remedied.

I'm afraid I'm not going to have time to look at this in detail anytime soon.

You might try changing SQLITE_STATIC to SQLITE_TRANSIENT here. That will cause an extra copy of the incoming ArrayBuffer (which shouldn't be necessary) but will make the potential race window smaller. (I don't currently have an environment set up where it would be easy for me to test that)

No, I don't think so. That issue would (I believe) let us remove this extra copy I noted here on reads, but this issue pertains to writes.

Note that the issue does also appear on reads (in fact reads appear to crash it more reliably)

Ah, got it, thanks for clarifying. Still, the API you mention would only affect reads; if it's occurring on writes without reads, that API wouldn't change anything here AFAICT.

Just as a thought, maybe this or this are missing move semantics. You can try to add a std::move in there, seems unlikely but memory handling is tricky

The problem was not reserving memory when copying the blob data. Awesome find my @amika-sq.

https://github.com/margelo/react-native-quick-sqlite/pull/11

I already copied the fix to op-sqlite. All new development will be on that package.