In https://phab.comm.dev/D4650 we enabled informative JSError to be thrown when SQLite query fails on databaseThread of CommCoreModule. I tested it only on emulators and it worked. However on physical devices I found out that I cannot access jsi::Runtime passed to function from auxiliary thread. Context is here https://github.com/react-native-community/discussions-and-proposals/issues/196. This diff updates the code so that we only catch C++ error in database thread and transform it to JSError on the main thread. This change also solves the crash described here: https://linear.app/comm/issue/ENG-1714/ashoat-experienced-5-crashes-in-one-day-on-build-142
Details
Details
Place temporary 'throw std::system_error(ECANCELED, std::generic_category(), "error");' at the begining of SQLiteWueryExecutor::getAllMessages. Build and start the app. Without this diff it will crash. With this diff it will throw informative JSError.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 168–173 ↗ | (On Diff #16048) | It seems like we have a similar approach in both places. Maybe we can create a function that takes a function or lambda and returns a generic T (so it probably should be a pointer to a function that takes no arguments and returns T)? We could use this util in both places. |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 168–173 ↗ | (On Diff #16048) | Or instead of pointer, we could use std::function |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 168–173 ↗ | (On Diff #16048) | In my honest opinion I would be an overkill to extract those try{}catch{}into functions. The logic here is not that complex, we are just storing error in a promise. Also if you look at the code of CommCoreModule.cpp you can see that there are multiple examples of try{}catch{} statements that look almost exactly the same - they differ only by the function executed inside try{}. In order to be consistent we would need to create such a function for every try{}catch{} pattern that is repeated at least twice since there is not reason to specially treat those two. |
Comment Actions
Initially we created this revision to do some code review before making the changes above part of the release build. Can I assume that we can land it now?