Details

Reviewers

marcin
• jon
tomek
atul

Commits

rCOMMafc218bf615f: [native] implement method to run synchronous cancellable task in CommCoreModule

Summary

context: ENG-2139

Introduced scheduleOrRunCancellable which will allow scheduling a task from the synchronous call that could be canceled while atomic flag in database singleton tasksCancelled will be set to true.

Using the error message as constant TASK_CANCELLED not a meaningful string because later in javascript we will need to indicate an error only by detecting if its message is equal to a given string or contain a given string.

Test Plan

Build app for both iOS and Android
Add changes with CommCoreModule methods which will simplify testing: D5582
Run this code to check if this works as supposed to:

try {
  commCoreModule.printMessageAndWaitSync('sync task 1');
} catch (e) {
  console.log('caught: ', e);
}
try {
  commCoreModule.printMessageAndWaitSync('sync task 2');
} catch (e) {
  console.log('caught: ', e);
}
try {
  await commCoreModule.cancelTasks();
} catch (e) {
  console.log('caught: ', e);
}
try {
  commCoreModule.printMessageAndWaitSync('sync task 3');
} catch (e) {
  console.log('caught: ', e);
}
try {
  commCoreModule.printMessageAndWaitSync('sync task 4');
} catch (e) {
  console.log('caught: ', e);
}
try {
  await commCoreModule.runTasks();
} catch (e) {
  console.log('caught: ', e);
}
try {
  commCoreModule.printMessageAndWaitSync('sync task 5');
} catch (e) {
  console.log('caught: ', e);
}

result:

2022-11-09 14:17:50.568369+0100 Comm[88845:5799175] COMM: Starting sync task: sync task 1
2022-11-09 14:17:50.572780+0100 Comm[88845:5799175] COMM: Ending sync task: sync task 1
2022-11-09 14:17:50.579646+0100 Comm[88845:5799175] COMM: Starting sync task: sync task 2
2022-11-09 14:17:50.582453+0100 Comm[88845:5799175] COMM: Ending sync task: sync task 2
2022-11-09 14:17:50.631433+0100 Comm[88845:5799137] [javascript] 'caught: ', [Error: Exception in HostFunction: TASK_CANCELLED]
2022-11-09 14:17:50.631909+0100 Comm[88845:5799137] [javascript] 'caught: ', [Error: Exception in HostFunction: TASK_CANCELLED]
2022-11-09 14:17:51.078927+0100 Comm[88845:5799175] COMM: Starting sync task: sync task 5
2022-11-09 14:17:51.082612+0100 Comm[88845:5799175] COMM: Ending sync task: sync task 5

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Nov 9 2022, 2:57 AM

kamil held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptNov 9 2022, 2:57 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

kamil edited the summary of this revision. (Show Details)Nov 9 2022, 3:06 AM

kamil edited the summary of this revision. (Show Details)Nov 9 2022, 3:08 AM

Harbormaster completed remote builds in B13307: Diff 18255.Nov 9 2022, 3:10 AM

kamil added a parent revision: D5576: [native] introduce atomic flag that indicates clearing database process will execute.Nov 9 2022, 3:40 AM

kamil added a child revision: D5578: [native] implement method to run asynchronous cancellable task in CommCoreModule and return error via promise.

kamil edited the test plan for this revision. (Show Details)Nov 9 2022, 5:19 AM

kamil published this revision for review.Nov 9 2022, 5:30 AM

ashoat added inline comments.Nov 9 2022, 6:00 AM

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
22	Are there really no other callsites we need to update from `scheduleOrRun` to `scheduleOrRunCancellable`?
native/ios/Comm/GlobalDBSingleton.mm
31	Shouldn't this be `scheduleOrRunCancellableCommonImpl`?

marcin added inline comments.Nov 10 2022, 5:08 AM

native/ios/Comm/GlobalDBSingleton.mm
31	It might be confusing here but we should use `scheduleOrRunCommonImpl` here. If we dispatch database task on the main queue it means that `GlobalDBSingleton` was called on iOS before JavaScript started (before `CommCoreModule` was created/before we enabled multithreading). Task cancellation takes place when `clearSensitiveData` is called in JavaScript which means we need to have `CommCoreModule` and multithreading enabled to call it.

marcin added inline comments.Nov 10 2022, 5:13 AM

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
24	I think the order here is confusing for the reader. It would be better to either move call to `task()` from line 38 to be below this if statement, or to move this if statement to be just above the call to `task()`.

ashoat added inline comments.Nov 10 2022, 5:18 AM

native/ios/Comm/GlobalDBSingleton.mm
31	Your point seems to be "there's no reason to make the task cancellable, since the only case for main-queue execution is on iOS before the JS thread starts, which means `clearSensitiveData` won't be called". But in the future, perhaps it's possible that some other code (eg. C++ code) will call `clearSensitiveData`. Is there any downside to using `scheduleOrRunCancellableCommonImpl` here?

marcin mentioned this in D5578: [native] implement method to run asynchronous cancellable task in CommCoreModule and return error via promise.Nov 10 2022, 5:23 AM

marcin added inline comments.Nov 10 2022, 5:27 AM

native/ios/Comm/GlobalDBSingleton.mm
31	There are not technical downsides of calling `scheduleOrRunCancellableCommonImpl` here. But I find it more consistent with our current logics.

ashoat added inline comments.Nov 10 2022, 5:41 AM

native/ios/Comm/GlobalDBSingleton.mm
31	I can't understand why you would think that. The method is called `scheduleOrRunCancellable`. It should clearly be calling `scheduleOrRunCancellableCommonImpl`...

kamil added inline comments.Nov 10 2022, 5:46 AM

native/ios/Comm/GlobalDBSingleton.mm
31	I've made a mistake here, I thought about this code in terms of "who will execute this task" but right now I see it's "who will schedule this" so I agree with @ashoat, I should have used `scheduleOrRunCancellableCommonImpl`

use scheduleOrRunCancellableCommonImpl, improve readabillity

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
22	Added later on this stack in D5579. I wanted to implement js handling first and spilt adding this function/using this function into separate diffs - there are a lot of changes to review.

Harbormaster completed remote builds in B13359: Diff 18316.Nov 10 2022, 7:36 AM

marcin added inline comments.Nov 10 2022, 8:34 AM

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
25 ↗	(On Diff #18316)	I think it would be highly beneficial to extract this string into some constant or static attribute of this class. It is repeated in many places (in this diff and in D5578).
26 ↗	(On Diff #18316)	I do not think this `return` is necessary. If you `throw` function execution is terminated.

reabse, introduce const, simplify logic

thanks for the suggestions @marcin, applied

Harbormaster completed remote builds in B13400: Diff 18375.Nov 11 2022, 5:55 AM

marcin accepted this revision.Nov 14 2022, 2:12 AM

marcin added a reviewer: tomek.

marcin added inline comments.

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
9 ↗	(On Diff #18375)	If you look through the codebase of C++ in native you will see that we follow a convention of initialization via curly brackets. Something like this: `const std::string TASK_CANCELLED_FLAG{"TASK_CANCELLED"};` I think we should follow it here as well.

Accepting, because my comments are only suggestions, but probably worth doing

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
9 ↗	(On Diff #18375)	https://www.notion.so/commapp/C-coding-standards-8c9f22b4b16a41c3a868eb5f537db1de
30–39 ↗	(On Diff #18375)	I'm not sure if I'm missing something, but it seems like we should use our existing logic
native/ios/Comm/GlobalDBSingleton.mm
13–31 ↗	(On Diff #18375)	It should be possible to reuse the logic a bit

This revision is now accepted and ready to land.Nov 14 2022, 9:19 AM

initialize via curly brackets, simplify logic

kamil added inline comments.Nov 16 2022, 2:33 AM

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
30–39 ↗	(On Diff #18375)	definitely, thanks for suggestion
native/ios/Comm/GlobalDBSingleton.mm
13–31 ↗	(On Diff #18375)	It is possible but it introduces another function, and in the next diff from the stack D5578 task type changes from `const std::function<void(taskType)>` because it takes additional arguments (`promise` and `jsInvoker`) which mean we will need to overlead `scheduleOrRunBase` or wrap this task in yet another lambda which after implementing this made feel confused wht's going on, but if you think it's still better I am okay doing this the way you suggested!