[native] Stop calling saveMessageInfos from androidBackgroundMessageTask
ClosedPublic
Actions

Authored by ashoat on Nov 18 2022, 5:16 AM.

Details

Reviewers

atul
tomek
marcin

Commits

rCOMMa00efd928866: [native] Stop calling saveMessageInfos from androidBackgroundMessageTask

Summary

androidBackgroundMessageTask runs separately from the main app, and doesn't have any mechanisms in place to wait for redux-persist and SQLite before dispatching Redux actions.

The result of this is that the store can be messed up. See ENG-2275 for details on how this can happen.

This diff removes the saveMessageInfos call from handleAndroidMessage, which gets called in two scenarios. The first scenario is androidBackgroundMessageTask, and in that scenario we don't want to call saveMessageInfos. The second scenario is PushHandler.androidMessageReceived, and in that scenario we'll continue calling saveMessageInfos.

Since the only component we need to call saveMessageInfos from now is PushHandler, I also replaced the hacky dispatch import approach we were taking before in push/utils.js with a more idiomatic approach.

Test Plan

Confirm that issue in ENG-2275 no longer repros

Diff Detail

Repository

rCOMM Comm

Branch

ashoat/androidNotifIssue

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Nov 18 2022, 5:16 AM

Harbormaster completed remote builds in B13546: Diff 18565.Nov 18 2022, 5:30 AM

ashoat requested review of this revision.Nov 18 2022, 5:30 AM

It makes sense to remove saveMessageInfos here, since native code should handle updating SQLite when notifs are received while the app is closed.

On the other hand, we can't remove the other dispatches here (see inline comments) as easily. I don't feel great about having any dispatches in androidBackgroundMessageTask, since it feels like they could potentially result in the same type of issue.

That said, I don't think we will see the issue in ENG-2275 from these actions, since the only reducer that looks at them is native/push/reducer.js.

We also know that redux-persist won't try to save data until it has finished rehydrating, which means the only thing we need to worry about is how these dispatches will function if the data from SQLite (threadStore, messageStore, and draftStore) are missing. I think the actions I point out in the inline comments should not cause any problems in that scenario.

Of course, if redux-persist isn't persisting these dispatches (eg. because rehydrate has not concluded), that also implies that the dispatches are being ignored, which means that we may fail to properly rescind notifs that we are displaying.

The long-term solution here is to move all of this to C++.

native/push/android.js
32	We still have a dispatch here
71	And here

ashoat mentioned this in D5668: [native] Replace hacky call to imported dispatch in intentionalSaveMedia.Nov 18 2022, 5:49 AM

ashoat added a child revision: D5668: [native] Replace hacky call to imported dispatch in intentionalSaveMedia.Nov 18 2022, 6:18 AM

ashoat mentioned this in D5671: [lib] Ensure messageStore.threads entry for each message from SQLite.Nov 19 2022, 12:03 PM

ashoat mentioned this in D5669: [lib] Populate messageStore.threads from threadInfos before setMessageStoreMessages.

This definitely solves the issue – just confirmed with some additional testing with Reactotron.

Separately, from reading the code it seems like CommNotificationsHandler is already updating SQLite in the background when a notif comes in. Wanted to check with @tomek, is this assessment correct? (I haven't actually tested the code yet to confirm)

In D5667#169021, @ashoat wrote:

This definitely solves the issue – just confirmed with some additional testing with Reactotron.

Separately, from reading the code it seems like CommNotificationsHandler is already updating SQLite in the background when a notif comes in. Wanted to check with @tomek, is this assessment correct? (I haven't actually tested the code yet to confirm)

It seems like we're accessing the db when a notif is received - I think that @marcin implemented that some time ago.

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

Ah yeah, you're right – looking at blame, it looks like @marcin added the code to save rawMessageInfos from notifs to SQLite on Android via CommNotificationsHandler.

@marcin, wanted to get your confirmation here – is this code currently working? Part of my reasoning for thinking it's okay to get rid of the saveMessageInfos here is that it is already handled in CommNotificationsHandler, so I wanted to make sure that is true.

This revision now requires review to proceed.Nov 21 2022, 8:21 AM

In D5667#169435, @ashoat wrote:

Ah yeah, you're right – looking at blame, it looks like @marcin added the code to save rawMessageInfos from notifs to SQLite on Android via CommNotificationsHandler.

@marcin, wanted to get your confirmation here – is this code currently working? Part of my reasoning for thinking it's okay to get rid of the saveMessageInfos here is that it is already handled in CommNotificationsHandler, so I wanted to make sure that is true.