Page MenuHomePhabricator
Differential D5594

[native] introduce error handling to cover cancelled database tasks
ClosedPublic
Actions

Authored by kamil on Nov 10 2022, 6:35 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 22, 10:56 PM
Unknown Object (File)
Feb 13 2025, 2:45 AM
Unknown Object (File)
Feb 13 2025, 2:45 AM
Unknown Object (File)
Feb 13 2025, 2:45 AM
Unknown Object (File)
Feb 13 2025, 2:45 AM
Unknown Object (File)
Feb 13 2025, 2:45 AM
Unknown Object (File)
Feb 13 2025, 2:44 AM
Unknown Object (File)
Feb 13 2025, 2:37 AM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
marcin
tomek
atul
Commits
rCOMM6c35669e5805: [native] introduce error handling to cover cancelled database tasks
Summary

context: ENG-2139

This code allows handling exceptions generated by canceling tasks (for now only when clearSensitiveData is called.

In the future maybe it will be cleaner and better to rely on the error code to avoid parsing the message but for now it's not possible, rejecting the promise allows passing only string link;

Test Plan
  1. Build app for both iOS and Android
  2. Add changes with CommCoreModule methods which will simplify testing: D5582
  3. Run code with bot sync and async code when tasks are canceled, surround with try {} catch.. to detect if checkIfTaskWasCancelled works correctly for both.
  4. Check if previous integrations with the database work properly eg. by adding some drafts etc.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil created this revision.Nov 10 2022, 6:35 AM
kamil held this revision as a draft.
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptNov 10 2022, 6:35 AM
Harbormaster completed remote builds in B13357: Diff 18314.Nov 10 2022, 6:49 AM
kamil edited the summary of this revision. (Show Details)Nov 10 2022, 6:53 AM
kamil edited the test plan for this revision. (Show Details)
kamil added a child revision: D5579: [native] refactor CommCoreModule to allow cancellable tasks.Nov 10 2022, 6:56 AM
kamil added a parent revision: D5578: [native] implement method to run asynchronous cancellable task in CommCoreModule and return error via promise.
kamil published this revision for review.Nov 10 2022, 7:41 AM
marcin added inline comments.Nov 10 2022, 9:22 AM
native/data/core-data-provider.react.js
37 ↗(On Diff #18314)

We have a Linear issue: https://linear.app/comm/issue/ENG-2128/display-alert-when-sensitivedatacleaner-is-activated that requires to display alert when database is deleted. Since task cancellation is closely related to database deletion, shouldn't we also display some kind if alert when such an exception is thrown?

native/data/sensitive-data-handler.react.js
33 ↗(On Diff #18314)

Is it possible that e will ever be TASK_CANCELLED here? Task cancellation occurs when we delete database. Database is deleted in this component. Database queries in this component are awaited sequentially before and after database deletion query. Not sue whether it is possible that database tasks issued inside this component can ever be cancelled.

native/data/sqlite-context-provider.js
66 ↗(On Diff #18314)

This will prevent SQLiteContextProvider from running its functionality until the app is restarted. Are we sure it is what we want?

native/utils/error-handling.js
3 ↗(On Diff #18314)

This flag is copied from C++ code introduced in previous differentials in this stack. Ideally it would be an attribute of GlobalDBSingleton that CommCoreModule would expose to JavaScript via some getter method. It would eliminate the coupling in the codebase. Nevertheless it is hard to estimate how much work it would be and this diff stack is a solution to a high priority Linear task so landing should not be blocked on implementing a getter method mentioned above.

kamil updated this revision to Diff 18405.Nov 14 2022, 2:00 AM

rebase

native/data/core-data-provider.react.js
37 ↗(On Diff #18314)

Because we have an alert when sensitive-data-cleaner is activated adding an alert here will result in two alerts at almost the same time, do we need this?
If yes could you create a task for that? I can add this in separate diff

native/data/sensitive-data-handler.react.js
33 ↗(On Diff #18314)

I was wondering if there will be any situation in the future when we will call clearSensitiveData from a different place in the code, in that scenario e.g. getCurrentUserID might fail, but if you are 100% sure clearSensitiveData will be called only here I can remove this.

native/data/sqlite-context-provider.js
66 ↗(On Diff #18314)

I added this because this was requested in the task description ENG-2139 (see 1.c) but actually I get your point. @ashoat do you have any thoughts on this?

native/utils/error-handling.js
3 ↗(On Diff #18314)

You're right it's not the best solution. I think exposing this flag through CommCoreModule for me doesn't look like the best solution either. I think we should find some workaround to make c++ code throw an exact instance of error and then do something like

catch (e instanceof TaskCancelled) {
   //
} catch (e) {
  ///
}

or at least use a unique error code but for now, I have no idea if it is possible. We can create a task and think about this later.

Harbormaster completed remote builds in B13419: Diff 18405.Nov 14 2022, 2:14 AM
marcin added inline comments.Nov 14 2022, 2:35 AM
native/data/core-data-provider.react.js
37 ↗(On Diff #18314)

I think you are right. Displaying an alert in this case wouldn't bring much more information than the alert we display in the case of database deletion.

native/data/sensitive-data-handler.react.js
33 ↗(On Diff #18314)

To be honest no one can be sure if and when we will decide to call clearSensitiveData fro other places in the code. But I recall @ashoat mentioning the possibility of calling clearSensitiveData from other places to justify usage of scheduleOrRunCancellableCommonImpl on iOS before JS thread starts. That said I agree with your point about introducing this check here.

marcin added inline comments.Nov 14 2022, 2:36 AM
native/utils/error-handling.js
3 ↗(On Diff #18314)

Creating a task for this is a good idea. You do not need to do it now, but please make sure you create it and link it in the diff prior to landing.

kamil added inline comments.Nov 14 2022, 3:42 AM
native/data/sensitive-data-handler.react.js
33 ↗(On Diff #18314)

Sorry, I forgot to mention this later but in fact, there is an exact situation when we will use clearSensitiveData in a different place - when database initialization will fail because we e.g. will be unable to perform database migration we will call this to re-create the database.

ashoat added inline comments.Nov 14 2022, 6:22 AM
native/data/sqlite-context-provider.js
66 ↗(On Diff #18314)

Yes, this is exactly what we want. If the database is being deleted, that means the user is being logged out. If the user is being logged out, we have nothing to fetch from SQLite.

It's honestly a bit concerning that this isn't clear to the team working on this at this point... I've written about it here, here, and in this comment:

Also note that SQLiteContextProvider should ALWAYS only run once, and should ALWAYS run exactly when the app starts. If the app starts while the user is logged in but then a logout is initiated, then SQLiteContextProvider might see a special exception thrown (see ENG-2139)… in that case, it should "give up" and just call setStoreLoaded(true).

It's critically important that the team here groks what we're doing and why. If you're basically treating this as "take Ashoat's requirements and implement them", then you will keep getting confused like this.

marcin accepted this revision.Nov 14 2022, 7:10 AM
marcin added a reviewer: tomek.
tomek accepted this revision.Nov 14 2022, 10:20 AM

In cases like this it is a good idea to include a description of what was done to make sure that we're updating all the relevant places, e.g. that a codebase was searched for all the usages of commCoreModule.

native/utils/error-handling.js
5 ↗(On Diff #18405)

I think that changing the name might make the usages more readable

This revision is now accepted and ready to land.Nov 14 2022, 10:20 AM
kamil updated this revision to Diff 18457.Nov 16 2022, 2:43 AM

rename funciton to isTaskCancelledError

kamil added a reviewer: atul.Nov 16 2022, 2:44 AM
This revision now requires review to proceed.Nov 16 2022, 2:44 AM
Harbormaster completed remote builds in B13458: Diff 18457.Nov 16 2022, 2:55 AM
atul removed 1 blocking reviewer(s): atul.Nov 17 2022, 9:22 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:22 AM
kamil updated this revision to Diff 18633.Nov 21 2022, 2:07 AM

rebase before landing

Harbormaster completed remote builds in B13604: Diff 18633.Nov 21 2022, 2:20 AM
Closed by commit rCOMM6c35669e5805: [native] introduce error handling to cover cancelled database tasks (authored by kamil). · Explain WhyNov 21 2022, 3:42 AM
This revision was automatically updated to reflect the committed changes.
kamil added a commit: rCOMM6c35669e5805: [native] introduce error handling to cover cancelled database tasks.

Revision Contents
Changeset List

PathSize
native/
data/
43 lines
28 lines
8 lines
redux/
21 lines
6 lines
utils/
9 lines

Diff 18457

View Options

native/data/core-data-provider.react.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

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?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

native/data/core-data.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

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?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

native/data/sqlite-context-provider.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

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?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

native/redux/persist.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

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?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

native/redux/redux-setup.js

Loading...
View Options

native/utils/error-handling.js

Loading...