If a cookie changes move the session from one place to a new one. If a cookie becomes null, we don't want to reassign - it might mean that e.g. a user logged out.
https://linear.app/comm/issue/ENG-7689/reassign-the-session
Depends on D11648
Details
Details
- Reviewers
kamil marcin varun - Commits
- rCOMM0fe1eb2d8e1d: [web] Reassign the notifs session
Open the web app, login, send a message from another client - a notif should be displayed (without this stack we can only see a notif with a warning message). Also, verify that the reassignment actually occurs by adding some console logs.
Opened two web tabs, and tried to log in on both of them. Sent a message from another client and confirmed that a notif was displayed.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I would amend to the test plan some steps that try to recreate race condition when we log in multiple pages at the same time. We should expect that after logging in to all pages we can see multiple notifs sessions in IndexedDB but when the notifs is received we can decrypt it and it removes all but one notif session.
| lib/components/keyserver-connection-handler.js | ||
|---|---|---|
| 69 ↗ | (On Diff #39085) | Excuse me if this is a silly question but I am still learning the ropes when it comes to React. Is there a risk of two reassignNotificationsSession promises running simultaneously? Shouldn't we keep a ref to the promise itself and set it to null once it finishes? |
Comment Actions
One additional questions:
Is it possible that due to some internal keyserver error the cookie will change and the keyserver will request the client to create new notifs session but new notifications session creation will execute before the effect is executed? In such case the old session would overwrite new one.
Comment Actions
Will do that! I included it in my final testing task.
Is it possible that due to some internal keyserver error the cookie will change and the keyserver will request the client to create new notifs session but new notifications session creation will execute before the effect is executed? In such case the old session would overwrite new one.
Maybe... it would require two things to happen almost at the same time: 1. keyserver sending new cookie 2. keyserver noticing that a notif session should be recreated. Going to think about it, but it sounds really unlikely to happen.
| lib/components/keyserver-connection-handler.js | ||
|---|---|---|
| 69 ↗ | (On Diff #39085) | In theory, there is a risk of this happening, but I'm not sure if it is real. |
Comment Actions
Is it possible that due to some internal keyserver error the cookie will change and the keyserver will request the client to create new notifs session but new notifications session creation will execute before the effect is executed? In such case the old session would overwrite new one.
Maybe... it would require two things to happen almost at the same time: 1. keyserver sending new cookie 2. keyserver noticing that a notif session should be recreated. Going to think about it, but it sounds really unlikely to happen.
After thinking about this it seems to be an extreme edge case. I think we can protect ourselves against it by reassigning only when the target item isn't set - that would mean that we created a new session for the new cookie. Unfortunately, this still leaves us with an even less probable race condition: localForage doesn't have an API that allows atomic "set if not present" operation, so it's still possible for an item to be set after we check and before we set a new one.
| lib/components/keyserver-connection-handler.js | ||
|---|---|---|
| 69 ↗ | (On Diff #39085) | Modified the solution so that the promises have a guaranteed order. |