Page MenuHomePhabricator

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

Authored by kamil on Nov 10 2022, 6:35 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jul 3, 6:23 PM
Unknown Object (File)
Wed, Jul 3, 8:52 AM
Unknown Object (File)
Wed, Jul 3, 7:49 AM
Unknown Object (File)
Tue, Jul 2, 8:26 PM
Unknown Object (File)
Tue, Jul 2, 10:30 AM
Unknown Object (File)
Mon, Jul 1, 12:14 AM
Unknown Object (File)
Sun, Jun 30, 7:41 AM
Unknown Object (File)
Wed, Jun 26, 9:59 PM

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil edited the test plan for this revision. (Show Details)
kamil published this revision for review.Nov 10 2022, 7:41 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.

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.

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.

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.

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.

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 added a reviewer: tomek.

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

rename funciton to isTaskCancelledError

This revision now requires review to proceed.Nov 16 2022, 2:44 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