This differential merges SensitiveDataHandler functionality into SQLiteContextProvider. There is a race condition between those two components which might lead to hard to debug issue. Therefore in this differential we make sure that functionalities privided by those two components are always executed in deterministic order.
Details
Details
Test plan for this differential should include test plans for those two differentials: https://phab.comm.dev/D4730, https://phab.comm.dev/D5444 since SensitiveDataHandler functionality should remain. Additionally we should temporarily add logging inside handleSensitiveData and before SQLiteContextProvider queries SQLite and updates redux. We should ensure the order of logs is deterministic on every app run.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I'm concerned about a potential scenario where the user is logged in when the app starts, but then quickly becomes logged out.
In this scenario, the commCoreModule data fetch will start (since the user is logged in when the app starts), but then the logout will trigger handleSensitiveData.
I could imagine this leading to a situation where the data fetch gets bad results because of handleSensitiveData, and then commits those bad results into Redux.
My suggestion to address this: the Redux action should have a field on it that says what the userID was when the fetch was initiated. Then the reducer can check if this matches the current userID, and if not it will ignore the data. This approach is similar to how invalidSessionDowngrade works.
That said, I think that should be done as part of ENG-2137. This diff I think is a clear improvement, so we can probably land it now.
I'd still like to get @tomek's and @atul's perspectives, however.
Comment Actions
Sounds like a valid concern and I agree that the solution will fix the issue. Also, landing this as is, is a good idea.