Page MenuHomePhabricator

[web] Reassign the notifs session
ClosedPublic

Authored by tomek on Apr 12 2024, 11:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 3, 7:07 AM
Unknown Object (File)
Oct 25 2024, 10:53 PM
Unknown Object (File)
Oct 12 2024, 9:05 PM
Unknown Object (File)
Oct 12 2024, 9:05 PM
Unknown Object (File)
Oct 12 2024, 9:05 PM
Unknown Object (File)
Oct 12 2024, 9:05 PM
Unknown Object (File)
Oct 12 2024, 9:05 PM
Unknown Object (File)
Oct 12 2024, 9:05 PM
Subscribers

Details

Summary

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

Test Plan

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

Repository
rCOMM Comm
Branch
notifs-init
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Set prev cookie immediately

Fix incorrect statements order

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?

This revision is now accepted and ready to land.Apr 15 2024, 12:33 AM

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.

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.

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.
In the most recent version, we're reassigning only when a new cookie is a user cookie, and this shouldn't happen frequently. Also, we're scheduling messages on our worker in sequence, so the first one should finish before the second one. I'm going to think a little more about this problem.

tomek marked an inline comment as done.

Add some protection against edge cases

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.

Always delete the old item

Rever changes to import ordering

This revision was automatically updated to reflect the committed changes.