Details

Reviewers

marcin
• jon
tomek
atul

Commits

rCOMM4cb87f3b1daf: [native] implement method to run asynchronous cancellable task in…

Summary

context: ENG-2139

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

In this scenario an error could not be thrown because it will kill the database thread, it has to be returned via promise and handled in js.

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 {
  await commCoreModule.printMessageAndWaitAsync('async task 1');
  await commCoreModule.printMessageAndWaitAsync('async task 2');
} catch (e) {
  console.log('caught: ', e);
}

try {
  await commCoreModule.cancelTasks();
} catch (e) {
  console.log('caught: ', e);
}

try {
  await commCoreModule.printMessageAndWaitAsync('async task 3');
  await commCoreModule.printMessageAndWaitAsync('async task 4');
} catch (e) {
  console.log('caught: ', e);
}

try {
  await commCoreModule.runTasks();
} catch (e) {
  console.log('caught: ', e);
}

try {
  await commCoreModule.printMessageAndWaitAsync('async task 5');
} catch (e) {
  console.log('caught: ', e);
}

result:

2022-11-09 14:24:10.079178+0100 Comm[88845:5799175] COMM: Starting async task: async task 1
2022-11-09 14:24:10.084171+0100 Comm[88845:5799175] COMM: Ending async task: async task 1
2022-11-09 14:24:10.097871+0100 Comm[88845:5799175] COMM: Starting async task: async task 2
2022-11-09 14:24:10.102069+0100 Comm[88845:5799175] COMM: Ending async task: async task 2
2022-11-09 14:24:10.114216+0100 Comm[88845:5799137] [javascript] 'caught: ', { message: 'TASK_CANCELLED' }
2022-11-09 14:24:10.115071+0100 Comm[88845:5799175] COMM: Starting async task: async task 5
2022-11-09 14:24:10.117837+0100 Comm[88845:5799175] COMM: Ending async task: async task 5

which demonstrates that we can not add a new task when taskCancelled: true, additionally we can remove await while calling commCoreModule.printMessageAndWaitAsync which will result that almost all (depending if the task will manage to execute) will throw an error that task was canceled - it's a scenario when tasks are in the queue but are removed without executing.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Nov 9 2022, 3:07 AM

kamil held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptNov 9 2022, 3:07 AM

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

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

Harbormaster completed remote builds in B13308: Diff 18256.Nov 9 2022, 3:19 AM

kamil added a parent revision: D5577: [native] implement method to run synchronous cancellable task in CommCoreModule.Nov 9 2022, 3:40 AM

kamil added a child revision: D5579: [native] refactor CommCoreModule to allow cancellable tasks.

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

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

Are there any callsites we need to update from scheduleOrRun to scheduleOrRunCancellable?

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
49 ↗	(On Diff #18256)	Can we reject with a `JSIError` instead of a `string`?
57–65 ↗	(On Diff #18256)	Early exit please
85 ↗	(On Diff #18256)	I would define this next to the other overload... doesn't make sense for `enableMultithreading` to be in between, in my opinion
native/ios/Comm/GlobalDBSingleton.mm
46 ↗	(On Diff #18256)	Shouldn't this be `this->scheduleOrRunCancellableCommonImpl(task, promise, jsInvoker)`?

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

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
57–65 ↗	(On Diff #18256)	Agree with @ashoat . The order here is confusing as in D5577
native/ios/Comm/GlobalDBSingleton.mm
46 ↗	(On Diff #18256)	Explanation here is the same as in D5577

kamil removed a child revision: D5579: [native] refactor CommCoreModule to allow cancellable tasks.Nov 10 2022, 6:56 AM

kamil added a child revision: D5594: [native] introduce error handling to cover cancelled database tasks.

use scheduleOrRunCancellableCommonImpl, improve readabillity

kamil added inline comments.Nov 10 2022, 7:41 AM

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
49 ↗	(On Diff #18256)	Unfortunately, I don't think so, this method accepts only string: docs
85 ↗	(On Diff #18256)	Agree, My oversight

Harbormaster completed remote builds in B13362: Diff 18319.Nov 10 2022, 7:53 AM

marcin mentioned this in D5577: [native] implement method to run synchronous cancellable task in CommCoreModule.Nov 10 2022, 8:34 AM

ashoat added inline comments.Nov 10 2022, 1:29 PM

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
49 ↗	(On Diff #18256)	Interesting!! So `jsi::JSError` can only be used in synchronous calls? Kind of strange, but okay

rebase, use const

Harbormaster completed remote builds in B13401: Diff 18376.Nov 11 2022, 6:02 AM

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

marcin added a reviewer: tomek.

marcin added inline comments.

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
47 ↗	(On Diff #18319)	Same as in D5577, this string is repeated so many times it should be extracted into some constant or static attribute.

Can we reduce the duplication of logic?

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
48 ↗	(On Diff #18376)	I don't think it is correct to capture `promise` by copy. Shouldn't we do this by reference? It feels like settling a copy of a promise shouldn't affect the original one...
57 ↗	(On Diff #18376)	Are we sure that we want to capture all these values by copy?

This revision now requires changes to proceed.Nov 14 2022, 9:42 AM

simplify logic

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

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
48 ↗	(On Diff #18376)	it's a pointer to `promise` so copying seems okay

kamil added a reviewer: atul.Nov 16 2022, 2:39 AM

Harbormaster completed remote builds in B13456: Diff 18455.Nov 16 2022, 2:50 AM

tomek accepted this revision.Nov 16 2022, 10:36 AM

tomek added inline comments.

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h
49–51 ↗	(On Diff #18455)	I don't think there's a reason behind using explicit captures in one lambda and default by copy in another. I think we should use consistent syntax to avoid confusion.
48 ↗	(On Diff #18376)	You're totally right, sorry for the confusion!

(Removing myself as blocking since I don't think I have enough context to give this a proper review)

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

rebase before landing, use explicit capture

Harbormaster completed remote builds in B13602: Diff 18631.Nov 21 2022, 2:19 AM

Closed by commit rCOMM4cb87f3b1daf: [native] implement method to run asynchronous cancellable task in… (authored by kamil). · Explain WhyNov 21 2022, 3:42 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM4cb87f3b1daf: [native] implement method to run asynchronous cancellable task in….

[native] implement method to run asynchronous cancellable task in CommCoreModule and return error via promise
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18649

native/android/app/src/cpp/GlobalDBSingleton.cpp

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h

native/ios/Comm/GlobalDBSingleton.mm

[native] implement method to run asynchronous cancellable task in CommCoreModule and return error via promiseClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18649

native/android/app/src/cpp/GlobalDBSingleton.cpp

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h

native/ios/Comm/GlobalDBSingleton.mm

[native] implement method to run asynchronous cancellable task in CommCoreModule and return error via promise
ClosedPublic
Actions

Revision Contents
Changeset List