Async JSI calls with ThreadPool and callback

[ospfranco]

No description provided.

[EduFrazao]

Its compiling here, but crashing on the JNI Adapter:

03-11 17:59:31.242 13476 13476 F DEBUG : Abort message: 'JNI DETECTED ERROR IN APPLICATION: use of invalid jobject 0x12f6fbc8
03-11 17:59:31.242 13476 13476 F DEBUG : from void com.reactnativequicksqlite.SequelModule.initialize(long, com.facebook.react.turbomodule.core.CallInvokerHolderImpl, java.lang.String)'

I will do some tests in this branch too. :)

[EduFrazao]

@ospfranco Finally I think I manage to get it working on Android!
I've made some tests with a simple test method, calling it a dozen times between many regular calls without a problem, already using your new thread pool implementation.

But I think that we will need to rework functions like sequel_execute.

This funcions receive and produce jsi::Object values, and in order to do that, they will need to do their works inside the JavaScript thread. When this functions do their work on the thread spawed by the pool causes erros.

Tomorrow I will rework the loadSQLFile function, because this feature can do the heavy work without touch jsi::Runtime. It will only need to use this object in order to produce the result status, and for this object its way more simple.
If it works fine I will submit my changes. Unless you prefer that I create a PR before.

[EduFrazao]

@ospfranco I belive that we will need to implement another function to use instead of "sequel_execute" in async operations.
I think that this version can maybe accept a vector or array of structs representing the dynamic values that we receive from user calls as jsi::Value objects, and it must return a dynamic datastructure that not depends on jsi::Runtime to be built.

In this way, we can execute sqlite operations in any thread without the need to synchronization for read or build jsi::Value objets. We can translate the parameters that users sent to us as arguments in the JavaScript caller thread, and then execute the operation in a task thread and use invokeAsync only to parse the results and translate it back to jsi objects to pass on the callback.

What do you think?

[ospfranco]

I take it you ran into the same issues I did. Which is namely, it looks like one cannot create jsi::Values from another thread and then return them in the callback. It's a bummer.

Indeed, the only way to move forward seems to me to refactor all the JSI code from the sqlite adapter code, my C++ is a bit too weak though, I started doing it and ran into many issues. I will continue through the day, but in any case, I do not want to have two different implementations (one depending on the JSI runtime object and one returning pure C++ data), I much rather do everything in C++ and convert the data at the end.

[ospfranco]

I got some of the dynamic types working, @EduFrazao maybe you can continue during the week, otherwise, I will continue working on it next weekend

[EduFrazao]

I got some of the dynamic types working, @EduFrazao maybe you can continue during the week, otherwise, I will continue working on it next weekend

Hi @ospfranco. I will check it right now. Thank you very mutch. I'm already using the async LoadSQLFile method and its working so nice :).
Sometime ago, before we got invokeAsync working, I was figuring out that we will need to refactor the database comunication layer because of this tight coupling with jsi objects, and I do some tests with a Struct + Enum to hold all the danyamic values. And it works, but I discarded this work because of problems with invokeAsync :). LOL.
But I will do some tests right now. Any news for the IOS?

[ospfranco]

ehm, yeah, I started refactoring the JSI code out of the sequel adapter, I will also do a major renaming of classes and stuff. but instead of using enums and structs I'm using C++ 17 any classes, don't know if might bring problems but it does make everything a lot easier to handle.

[EduFrazao]

ehm, yeah, I started refactoring the JSI code out of the sequel adapter, I will also do a major renaming of classes and stuff. but instead of using enums and structs I'm using C++ 17 any classes, don't know if might bring problems but it does make everything a lot easier to handle.

Yeah, code looks cleaner. I will do some tests with your approach and compare with "classical" structs and try to migrate my whole App do fully use async executeSql commands :).

[EduFrazao]

Hi @ospfranco .
I've refactored the LoadSQLFile method, and also the SQLBatchExecution. And I also added a async version of this feature.
Opened PR #32

[EduFrazao]

@ospfranco I tested this changes with my current application, using sync and async methods, with and without hermes, and no problems so far.
Tested on Android 11 emulator and in a real device.

On IOS I can only test on the emulator.

[ospfranco]

Thanks Edu, I still want to get rid of the duplicated execute_query methods and have only one

[ospfranco]

@EduFrazao I removed the execute_sequel method that contained JSI code, would you mind testing it again on your methods and making sure everything is working, it would also be good if you can add example/test methods in the Database.ts file in the example project for people to know how to use these

[EduFrazao]

@EduFrazao I removed the execute_sequel method that contained JSI code, would you mind testing it again on your methods and making sure everything is working, it would also be good if you can add example/test methods in the Database.ts file in the example project for people to know how to use these

Very nice. I wil run my tests and try to test on IOS too.
Count on me to add add tests and examples to Database.ts too

[EduFrazao]

@EduFrazao I removed the execute_sequel method that contained JSI code, would you mind testing it again on your methods and making sure everything is working, it would also be good if you can add example/test methods in the Database.ts file in the example project for people to know how to use these

Tested on Android 11 Emulator and physical device
Tested on IOS 15 Emulator

Tested methods:

executeSql, asyncExecuteSql
loadSQLFileAsync, executeSqlBatch, asyncExecuteSqlBatch

No issues observed.
I will try do deploy on a IOS physical device today.

[ospfranco]

@EduFrazao great, thanks. Would you mind taking care of the last review comment, trying to reduce the number of gradle dependencies?

[EduFrazao]

@EduFrazao great, thanks. Would you mind taking care of the last review comment, trying to reduce the number of gradle dependencies?

Sure, I will do some tests tonight.

[EduFrazao]

@EduFrazao great, thanks. Would you mind taking care of the last review comment, trying to reduce the number of gradle dependencies?

Done. PR #34 opened.