Page MenuHomePhabricator
Differential D5577

[native] implement method to run synchronous cancellable task in CommCoreModule
ClosedPublic
Actions

Authored by kamil on Nov 9 2022, 2:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 29, 9:39 PM
Unknown Object (File)
Sun, Mar 23, 4:35 PM
Unknown Object (File)
Tue, Mar 11, 3:49 PM
Unknown Object (File)
Thu, Mar 6, 2:22 PM
Unknown Object (File)
Thu, Mar 6, 1:52 PM
Unknown Object (File)
Feb 17 2025, 6:33 AM
Unknown Object (File)
Feb 13 2025, 2:45 AM
Unknown Object (File)
Feb 13 2025, 2:45 AM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

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
  1. Build app for both iOS and Android
  2. Add changes with CommCoreModule methods which will simplify testing: D5582
  3. 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
Branch
clear-db-queue
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 ↗(On Diff #18255)

Are there really no other callsites we need to update from scheduleOrRun to scheduleOrRunCancellable?

native/ios/Comm/GlobalDBSingleton.mm
31 ↗(On Diff #18255)

Shouldn't this be scheduleOrRunCancellableCommonImpl?

marcin added inline comments.Nov 10 2022, 5:08 AM
native/ios/Comm/GlobalDBSingleton.mm
31 ↗(On Diff #18255)

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 ↗(On Diff #18255)

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 ↗(On Diff #18255)

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 ↗(On Diff #18255)

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 ↗(On Diff #18255)

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 ↗(On Diff #18255)

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

kamil updated this revision to Diff 18316.Nov 10 2022, 7:22 AM

use scheduleOrRunCancellableCommonImpl, improve readabillity

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
22 ↗(On Diff #18255)

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

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

I do not think this return is necessary. If you throw function execution is terminated.

kamil updated this revision to Diff 18375.Nov 11 2022, 5:42 AM

reabse, introduce const, simplify logic

kamil added a comment.Nov 11 2022, 5:44 AM

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.

tomek accepted this revision.Nov 14 2022, 9:19 AM

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
kamil updated this revision to Diff 18454.Nov 16 2022, 2:25 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!

kamil added a reviewer: atul.Nov 16 2022, 2:36 AM
This revision now requires review to proceed.Nov 16 2022, 2:36 AM
Harbormaster completed remote builds in B13455: Diff 18454.Nov 16 2022, 2:38 AM
atul removed 1 blocking reviewer(s): atul.Nov 17 2022, 9:16 AM

(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:16 AM
kamil updated this revision to Diff 18630.Nov 21 2022, 2:04 AM

rebase before landing

Harbormaster completed remote builds in B13601: Diff 18630.Nov 21 2022, 2:18 AM
Closed by commit rCOMMafc218bf615f: [native] implement method to run synchronous cancellable task in CommCoreModule (authored by kamil). · Explain WhyNov 21 2022, 3:42 AM
This revision was automatically updated to reflect the committed changes.
kamil added a commit: rCOMMafc218bf615f: [native] implement method to run synchronous cancellable task in CommCoreModule.

Revision Contents
Changeset List

PathSize
native/
android/
app/
src/
cpp/
4 lines
cpp/
CommonCpp/
NativeModules/
2 lines
InternalModules/
19 lines
ios/
Comm/
11 lines

Diff 18316

View Options

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

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

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

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/ios/Comm/GlobalDBSingleton.mm

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };