OP-Engineering / op-sqlite

Fastest SQLite library for react-native by @ospfranco
MIT License
593 stars 41 forks source link

Calling .close() closes all existing DB Connections? #146

Closed On2it-Clifford closed 2 months ago

On2it-Clifford commented 2 months ago

Describe the bug

let db1 = open({ name: 'test' });
let db2 = open({ name: 'test' });

db1.close();
db2.execute('Anything');

db2 will throw 'Error: Exception in HostFunction: [OP-SQLite] DB is not open' exception even thou I closes only the first connection.

Versions:

ospfranco commented 2 months ago

No, this is a bug, it's unlikely you need two connections to the same db in your JS but I will take care of it anyways

ospfranco commented 2 months ago

Allowing multiple clean connections to the DB would require far more refactoring than I have time for. I have now disabled it. If you try to open a new connection to the same DB in JS it will throw an exception.

It will be released with the next major version.

https://github.com/OP-Engineering/op-sqlite/pull/142

On2it-Clifford commented 2 months ago

Hi @ospfranco, I understand what you meant. But the main reason I asked for this is because I have multiple backend jobs running at the same time. Example: Scheduler A: Every 5 seconds will query Table A and do some task Scheduler B: Every 5 seconds will query Table B and do some task

The problem is that occasionally, when both task run at the same time, one of the scheduler will closes the other scheduler's connection when it completes it task by calling .close, and this will cause the other scheduler that has yet to complete its task to have it's DB connection closed due to calling .close will close every single opened DB connection.

My example posted there is just to have a simple test case to replicate the issue that I am currently facing right now. My use case is far more complicated than the example I posted here.

ospfranco commented 2 months ago

You don't need multiple connections for this. Just use executeAsync, tasks run on separate threads. No need to open/close the db every single time.

ospfranco commented 2 months ago

On the new version that will be released soon, you just need execute which is now async, there are no more sync versions.

On2it-Clifford commented 2 months ago

Yes @ospfranco, initially I do not close db connection and reuse same db connection, it is working fine. But this will cause memory leak, where each query will consume the memory and does not release it if I do not call .close() and eventually it will crash the app when the memory is too low.

On2it-Clifford commented 2 months ago

You can do a simple test where you open a db connection and use a while(true) loop that constantly query something(anything simple is applicable) using the same db connection without .close(), the memory never get released until we kill the app.

ospfranco commented 2 months ago

Interesting, the new version should help maybe. Objects returned by the normal execute are no longer HostObjects so they can be properly cleaned. Give the 8.0.0-beta0 a try and if not open a new issue. No memory leaks should happen.

On2it-Clifford commented 2 months ago

Alright @ospfranco I will test it out and let you know again, thanks.

ospfranco commented 2 months ago

8.0.1 is out, give that one a try

On2it-Clifford commented 2 months ago

Just had a quick test on 8.0.0-beta0, seems to be working very fine with no leaks at all. Will try with 8.0.1 again later. Thanks for the quick response.

0cothucluc commented 1 week ago

@ospfranco I'm facing [Error: Exception in HostFunction: [OP-SQLITE] Only one connection per database is allowed, db name: abc, when I open app again, it sometimes throw the exception, what is the best practice for this case? image

ospfranco commented 1 week ago

Don't close the connection, just create one per app start

0cothucluc commented 1 week ago

so I just need to open the connection when app starts right? Edit: when I dont close the connection, the exception still there

ospfranco commented 1 week ago

Read the docs, you also need to await the execute call

0cothucluc commented 1 week ago

I added .then() for execute call The docs u just say use .close to close the connection

ospfranco commented 1 week ago

Your app initialization code is wrong. When your app starts/hot reload is trying to create the DB connection again, which is wrong. I cannot tell you what's wrong because I don't have access to your code. Just create the connection once. Don't put it in a function just in a db.ts file create it at the module level:

// db.ts
export const db = open({...})

Read the docs carefully.

0cothucluc commented 1 week ago

I got it, I put .open in useEffect(), thx for fast reply