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
- Branch
- marcin/eng-1714
- 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?
For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?