Async Execution Errors: 2nd Error Overrides First

[savv]

Hi Franco, thanks again for your recent fix that destructed ThreadPools upon restart! I noticed another minor threading issue that I wanted to report. I don't think it needs to be fixed with any urgency, but I still thought it worth reporting here, as it took some digging to figure it out, and may help uncover other latent issues.

When running multiple async executions in parallel, I hit a race condition where I'm receiving the error of another execution, actually receiving [react-native-quick-sqlite] SQL execution error: not an error which corresponds to SQLITE_OK. I think what happened is what's described at the bottom here:

When the serialized threading mode is in use, it might be the case that a second error occurs on a separate thread in between the time of the first error and the call to these interfaces. When that happens, the second error will be reported since these interfaces always report the most recent result. To avoid this, each thread can obtain exclusive use of the database connection D by invoking sqlite3_mutex_enter(sqlite3_db_mutex(D)) before beginning to use D and invoking sqlite3_mutex_leave(sqlite3_db_mutex(D)) after all calls to the interfaces listed here are completed.

[ospfranco]

You are still using quick-sqlite, please migrate to op-sqlite. I added the error code to the error output, and not an error is not always SQLITE_OK, it can be SQLITE_MISSUSE, which means your SQL query is not properly supported by sqlite.

[savv]

I added the error code to the error output,

Nice, I can see it, thanks! It may be worth to call sqlite3_errmsg at the point where you set is_failed = true. This might increase the changes of capturing the right error, since the intervening sqlite3_finalize may cause the thread to get preempted (and thus receive the wrong error). That may be simpler than figuring out the performance implications of sqlite mutexes.

and not an error is not always SQLITE_OK, it can be SQLITE_MISSUSE, which means your SQL query is not properly supported by sqlite.

In sqlite3.c, when I look at the implementation for sqlite3ErrStr(int rc){, I see the following:

  static const char* const aMsg[] = {
    /* SQLITE_OK          */ "not an error",
    // ...
    /* SQLITE_MISMATCH    */ "datatype mismatch",
    /* SQLITE_MISUSE      */ "bad parameter or other API misuse",

You are still using quick-sqlite, please migrate to op-sqlite.

We are planning to but ran into a couple of issues that I haven't had the time to properly dive into and report.

[ospfranco]

I added the error code to the error output,

Nice, I can see it, thanks! It may be worth to call sqlite3_errmsg at the point where you set is_failed = true. This might increase the changes of capturing the right error, since the intervening sqlite3_finalize may cause the thread to get preempted (and thus receive the wrong error). That may be simpler than figuring out the performance implications of sqlite mutexes.

Ah, I see what you mean, unfortunately, while this might make it somewhat better is still not correct. I will do the change, but I don't want to lock the database with mutexes on the native level, code will get too complex to fast. What you really want to do is do all your operations with transactions, the mutex is already implemented at the JS level and you should be able to get the correct error codes.

and not an error is not always SQLITE_OK, it can be SQLITE_MISSUSE, which means your SQL query is not properly supported by sqlite.

In sqlite3.c, when I look at the implementation for sqlite3ErrStr(int rc){, I see the following:

  static const char* const aMsg[] = {
    /* SQLITE_OK          */ "not an error",
    // ...
    /* SQLITE_MISMATCH    */ "datatype mismatch",
    /* SQLITE_MISUSE      */ "bad parameter or other API misuse",

Ah, you are right, weird, I was debugging another issue yesterday and I got SQLITE_MISSUSE with not an error description.

[savv]

Ah, I see what you mean, unfortunately, while this might make it somewhat better is still not correct.

Yes, moving sqlite3_errmsg to the point where you set is_failed = true is not 100% correct.

I will do the change, but I don't want to lock the database with mutexes on the native level, code will get too complex to fast.

I agree.

What you really want to do is do all your operations with transactions, the mutex is already implemented at the JS level and you should be able to get the correct error codes.

I couldn't immediately see how transactions could fix the issue. A counterexample: you can have two single async queries, throwing two different errors, and, with this problem, you will still get the wrong error for each.

Ah, you are right, weird, I was debugging another issue yesterday and I got SQLITE_MISSUSE with not an error description.

Assuming that they were async, I think you were seeing the error that I'm describing:

• one execution had a syntax error and returned (which you kept in a variable with result = sqlite3_step(statement);)
• the other async execution was successful (SQLITE_OK)
• when you created .message you used the result codeas well as the error description from the other execution that had returned SQLITE_OK.

[ospfranco]

What you really want to do is do all your operations with transactions, the mutex is already implemented at the JS level and you should be able to get the correct error codes.

I couldn't immediately see how transactions could fix the issue. A counterexample: you can have two single async queries, throwing two different errors, and, with this problem, you will still get the wrong error for each.

Transactions are implemented with a mutex on the JS level

Ah, you are right, weird, I was debugging another issue yesterday and I got SQLITE_MISSUSE with not an error description.

Assuming that they were async, I think you were seeing the error that I'm describing:

• one execution had a syntax error and returned (which you kept in a variable with result = sqlite3_step(statement);)
• the other async execution was successful (SQLITE_OK)
• when you created .message you used the result codeas well as the error description from the other execution that had returned SQLITE_OK.

Yeah I got how the current error happens. That wasn't the case yesterday though.

[ospfranco]

Going to close this for now, as I don't plan to implement threading safe code on the native side. The recommended solution is never to do insertions in parallel using pure executeAsync calls. One should always use transactions that already have a mutex implemented (on JS).

[savv]

Hi Oscar, in my opinion it is a shame not to benefit from a phone's multiple cores and, therefore, at least in our case, I would stick with executeAsync.

Therefore, I think it is still worth it to call sqlite3_errmsg at the point where you set is_failed = true, as it will at least significantly increase the chances of capturing the right error. But I leave it to your judgement, we can also implement this on our side.

[ospfranco]

calling sqlite3_errmsg at the earliest point is already done.