[lib] Fix order of resolving keyserver calls after session recovery
ClosedPublic
Actions

Authored by ashoat on Jan 18 2024, 8:52 AM.

Details

Reviewers

tomek
inka
atul

Commits

rCOMMa53d9f43351c: [lib] Fix order of resolving keyserver calls after session recovery

Summary

While testing, noticed that the second message would get sent after the first after session recovery. This diff makes sure we resolve the first message (that triggers the invalidation) before the second.

Depends on D10696

Test Plan

I tested this stack using the following procedure:

I tested primarily on native
1. I compiled a dev build and deployed it to an iOS simulator
2. I created a brand new account on my local keyserver using the iOS app
3. I ran Redux dev tools: cd native && yarn redux-devtools
4. I added a 30s sleep at the start of resolveKeyserverSessionInvalidation
5. I made KeyserverConnectionsHandler return null so that the socket wouldn’t automatically recover the session prior to my testing
6. I killed the app
7. I deleted all of the test user’s cookie
8. I then opened the app again and navigated to a chat and sent two messages
9. By following the Redux monitor, I was able to see that the keyserver session invalidation recovery was successful, and both messages were eventually sent after the 30s sleep concluded
On web, we don’t support keyserver session invalidation. However, I tested to make sure that the web app still loaded after my changes

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Jan 18 2024, 8:52 AM

Harbormaster completed remote builds in B25873: Diff 35768.Jan 18 2024, 9:16 AM

ashoat requested review of this revision.Jan 18 2024, 9:16 AM

tomek accepted this revision.Jan 18 2024, 3:08 PM

tomek added inline comments.

lib/keyserver-conn/call-keyserver-endpoint-provider.react.js
163–174 ↗	(On Diff #35768)	There are some issues with async promise executor https://eslint.org/docs/latest/rules/no-async-promise-executor. It seems like we can avoid the warning

This revision is now accepted and ready to land.Jan 18 2024, 3:08 PM

ashoat added inline comments.Jan 18 2024, 5:50 PM

lib/keyserver-conn/call-keyserver-endpoint-provider.react.js
163–174 ↗	(On Diff #35768)	I read through that doc and feel like its concerns didn't apply in my case (I catch promise rejections, and am using the `async` in an intentional way). That said, your approach avoids the warning, which is probably a good enough reason to do it

ashoat added inline comments.Jan 18 2024, 6:14 PM

lib/keyserver-conn/call-keyserver-endpoint-provider.react.js
163–174 ↗	(On Diff #35768)	I tried your suggested approach, but for some reason it did not appear to resolve the initial issue... I think it has to do with the nuances of making `attemptToResolveInvalidation` an `async` function, but I didn't have time to investigate this more. Will leave this as-is, except for removing the useless `return` on line 185

Remove useless return

ashoat added inline comments.Jan 18 2024, 6:17 PM

lib/keyserver-conn/call-keyserver-endpoint-provider.react.js
163–174 ↗	(On Diff #35768)	I think it has to do with the nuances of making `attemptToResolveInvalidation` an `async` function, but I didn't have time to investigate this more. Will leave this as-is, except for removing the useless `return` on line 185 Actually, I don't think it's that... I accidentally ended up testing with the `async` keyword still there, and it appeared to work. I guess maybe it is "optimized away" unless I additionally move the `await attemptToResolveInvalidationHelper(sessionChange)`?