Page MenuHomePhabricator

[lib] Cancel an auth every time dependencies change
ClosedPublic

Authored by tomek on Jan 31 2024, 7:29 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 25, 5:12 AM
Unknown Object (File)
Mar 12 2024, 1:47 AM
Unknown Object (File)
Mar 10 2024, 9:20 PM
Unknown Object (File)
Mar 10 2024, 2:53 PM
Unknown Object (File)
Mar 10 2024, 1:02 PM
Unknown Object (File)
Feb 20 2024, 8:58 PM
Unknown Object (File)
Feb 20 2024, 8:54 PM
Unknown Object (File)
Feb 20 2024, 8:27 PM
Subscribers

Details

Summary

Extract a callback from the effect. This callback returns a promise and a function that can cancel it.

Cancelling a promise is implemented by checking the flag and throwing an exception if a flag is true.

The effect depends on a callback and on canPerformAuth flag, which handles a retries delay. When the callback changes, it means that some of its deps changed and we should start a new auth. The first operation in the callback is to set canPerformAuth to false, which then would result in the effect being fired, but the callback itself doesn't depend on this flag. That allows us to easily check if we need to cancel just by comparing the callback with its previous value.

Depends on D10844

Test Plan

Comment out checking usingCommServicesAccessToken and isUserAuthenticated flags. Added a lot of console logs and some sleeps to make sure that canceling an ongoing auth results in throwing an exception and restarts an auth.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Jan 31 2024, 7:48 AM
lib/components/keyserver-connection-handler.js
199 ↗(On Diff #36384)

This is the only change that modifies the logic - we don't want to auth when we don't have a token yet.

I am probably a good person to review this diff, since I suggested the cancel approach

ashoat requested changes to this revision.Jan 31 2024, 10:12 AM

I have some questions

lib/components/keyserver-connection-handler.js
89 ↗(On Diff #36384)

I wonder if this should be renamed to authInProgress to improve readability. This is technically a bit inaccurate, as the "delay" at the end is not really part of auth... but in my opinion, it's more clear. Otherwise when I first see this, I'm thinking it's a permissions thing, or something to do with required data being available

95 ↗(On Diff #36384)

When cancel is called, we currently need to wait for one of the if (cancelled) lines below before setCanPerformAuth(true) is run.

As a result, when something changes, a second auth has to wait a little before it can start.

Is there a downside to immediately starting the second auth? What would happen if we added setCanPerformAuth(true) here, removed line 162, and made line 178 conditional on cancelled being false?

162 ↗(On Diff #36384)

Nit: is this line necessary? I think the finally block will still execute before the return, but I may be wrong

205 ↗(On Diff #36384)

If for some reason the access token disappears from Redux (for instance, an identity session invalidation), should we also cancel a pending auth?

210–220 ↗(On Diff #36384)

I'm not sure I understand the need for the prevPerformAuth ref.

What would be wrong with this code?

React.useEffect(() => {
  if (
    !usingCommServicesAccessToken ||
    isUserAuthenticated ||
    !hasAccessToken
  ) {
    return;
  }
  cancelPendingAuth.current?.();
  cancelPendingAuth.current = null;
  if (!canPerformAuth) {
    return;
  }
  const [, cancel] = performAuth();
  cancelPendingAuth.current = cancel;
}, [canPerformAuth, hasAccessToken, isUserAuthenticated, performAuth]);

Is the concern that perhaps one of the boolean deps could flip twice, resulting in a login being cancelled unnecessarily?

211 ↗(On Diff #36384)

I wonder if we should set cancelPendingAuth.current = null here. I don't think it's very important, but seems more "correct", to avoid cancelling multiple times (which should be a no-op)

This revision now requires changes to proceed.Jan 31 2024, 10:12 AM
tomek marked 5 inline comments as done.

Improve canceling logic

tomek marked an inline comment as done.

Rename

lib/components/keyserver-connection-handler.js
89 ↗(On Diff #36384)

Makes sense

95 ↗(On Diff #36384)

I think this should work, and will be significantly more efficient when handling a delay

162 ↗(On Diff #36384)

Right, we can remove it. But early return should stay, to make sure that we don't log the user out when canceling.

205 ↗(On Diff #36384)

It might be a better and safer approach

210–220 ↗(On Diff #36384)

The first thing we're doing in performAuth() is setting canPerformAuth flag. Changing this flag would result in this effect being run (it depends on canPerformAuth flag) which would cancel the auth. Then the flag would flip again, the effect would start, etc. causing an infinite loop.

performAuth function depends on a lot of things that affect the auth, but it doesn't depend on canPerformAuth flag. Comparing it with a prev value allows us to easily differentiate between a case where we've just started the auth from a case where something important changed.

211 ↗(On Diff #36384)

Ok, we can do that

ashoat added inline comments.
lib/components/keyserver-connection-handler.js
89 ↗(On Diff #36513)

I think we should invert the true / false here. true for "can perform auth" corresponds to false for "auth in progress"

210–220 ↗(On Diff #36384)

Thanks for explaining!

This revision is now accepted and ready to land.Feb 1 2024, 9:08 AM
tomek marked an inline comment as done.

Invert the flag

lib/components/keyserver-connection-handler.js
89 ↗(On Diff #36513)

Yeah, definitely!

lib/components/keyserver-connection-handler.js
101–127 ↗(On Diff #36536)

Can all three of these awaits be combined into one?

Await more promises at once

lib/components/keyserver-connection-handler.js
101–127 ↗(On Diff #36536)

Yes, we can do that

lib/components/keyserver-connection-handler.js
101–102

Why wasn't this await combined with the next ones?

lib/components/keyserver-connection-handler.js
101–102

The result of getKeyserverKeys call is used as an input to the next calls (notificationsSessionCreator and contentSessionCreator)

lib/components/keyserver-connection-handler.js
101–102

Oops, I missed that. Thanks for explaining!