Support concurrent requests in `retryWithRelogin` by gzdunek · Pull Request #35599 · gravitational/teleport

gzdunek · 2023-12-11T11:51:22Z

This PR adds handling concurrent requests in retryWithRelogin.

Currently, when two requests want to show a login modal, this happens: the first request shows the modal and makes the request wait for it to resolve. Then another request hides the previous login modal and shows a new one. The previous request will never resolve.

The PR fixes that by storing the login promise, so the concurrent requests can wait for it to finish.

This fix will be needed for #35251 where we send two requests at the same time: for resources and for preferences.

ravicious · 2023-12-14T14:48:40Z

+  if (!pendingLogin) {
+    pendingLogin = login(appContext, rootClusterUri).finally(() => {
+      pendingLogin = undefined;
+    });
+  }


This doesn't handle well two concurrent requests from two different workspaces. Let's handle this scenario and add a test case for it.

At the moment if workspace B calls retryWithRelogin while the call from workspace A is in progress, the call from workspace B will wait for the call from A to resolve, after which it'll call actionToRetry() again. It'll fail because we didn't attempt to log in to workspace B, we just waited for the workspace A to finish its login ceremony.

I think the way to address this is to change retryWithRelogin so that if there's a pending login, it should wait for the login to finish. Then it should check if a relogin is still needed and if so, show the modal again.

There are two problems though:

At the moment the only way retryWithRelogin knows whether a relogin is needed is when actionToRetry returns a retryable error.

This is not ideal because we want to know this without retrying an action.

We cannot depend on the state of ClustersService, because cluster.connected does not get flipped to false after the cert expires.

Perhaps we could do a quick tshd.getCluster to check if the cert is valid? This doesn't make any network requests and returns current connected so it should be quick.

If there's one login in progress and two retryWithRelogin waiting for it to finish, they cannot both just wait for the login to finish and then show the modal at the same time. The second one to run would just override the other one that was waiting.

One way to solve it would be to make a recursive call to retryWithRelogin if we fetch fresh connected and deem that a relogin is still necessary, instead of just calling actionToRetry. We could still call actionToRetry if it turned out that relogin is not necessary anymore.

If we addressed 1 and 2, if there were two concurrent calls from the same workspace and the user canceled the first one, the second one would show a login modal again. I feel like in this situation it should just fail.

I guess on top of storing pendingLogin, we could also store which workspace this pendingLogin is related to by storing the root cluster URI. Then if we see that the URI is the same, the code can use the current logic from your PR. If the URIs are different, it should trigger the logic with showing another login modal.

Mind you that here you'd have the same problem with showing two modals in quick succesion that we had when working with Brian to implement headless login.

Thanks for the deep analysis!

The code I added indeed doesn't support the scenario with multiple workspaces, but I thought it wasn't really necessary.

Before we make an attempt to relogin in retryWithRelogin (which shows a modal), we do this:

if (!workspacesService.doesResourceBelongToActiveWorkspace(resourceUri)) { // The error is retryable, but by the time the request has finished, the user has switched to // another workspace so they're no longer looking at the relevant UI. // // Since it might take a few seconds before the execution gets to this point, the user might no // longer remember what their intent was, thus showing them a login modal could be disorienting. // // In that situation, let's just not attempt to relogin and instead let's surface the original // error. throw retryableErrorFromActionToRetry; }

Doesn't it solve our problem in the first place?
If you run an action while you are in a workspace A, it will fail without showing a login modal if you have switched to workspace B.

If I'm correct, the only situation in which we would have to deal with handling concurrent requests from different workspaces would be the following scenario:

The request from workspace A triggers a login modal, pendingLogin is set. You don't dismiss it.

You switch to the workspace B and run a request that triggers its own login modal. Now the request would wait for the modal from the workspace A.

But should we assume that the user can change a workspace when the login modal is visible? I think it's not a realistic scenario.

I think that makes sense.

This made me realize that if a relogin modal is shown and you open a deep link to another workspace and that deep link triggers a login modal, then the relogin attempt will just hang forever.

I guess now that we have this pendingLogin, maybe we could actually make it so that openRegularDialog/openImportantDialog cancels the previous dialog when opening a new one?

But also feel free to ignore it as it goes a little bit out of scope of this PR, which was made to solve the issue at hand with concurrent requests in unified resources.

Good idea, it shouldn't have any negative side effects. I clicked through the app and everything seems to work fine.

I did some tests too and everything appears to be working fine. 👍

ravicious · 2023-12-14T14:50:59Z

+    });
+  }
+
+  await pendingLogin;


I feel like this only works because we know that pendingLogin never rejects. Otherwise if there was a pending login, the second call would need to wait for pendingLogin to finish but it should ignore any errors.

But as I said, pendingLogin never rejects anyway. It's just something that raised my eyebrows. Maybe worth a comment.

Maybe I should call it pendingLoginDialog? It would suggest that the promise is tied to a UI element (instead of an API call) so it won't reject.

Good idea. 👍

ravicious · 2023-12-15T15:35:01Z

+  if (!pendingLogin) {
+    pendingLogin = login(appContext, rootClusterUri).finally(() => {
+      pendingLogin = undefined;
+    });
+  }


I think that makes sense.

This made me realize that if a relogin modal is shown and you open a deep link to another workspace and that deep link triggers a login modal, then the relogin attempt will just hang forever.

I guess now that we have this pendingLogin, maybe we could actually make it so that openRegularDialog/openImportantDialog cancels the previous dialog when opening a new one?

But also feel free to ignore it as it goes a little bit out of scope of this PR, which was made to solve the issue at hand with concurrent requests in unified resources.

ravicious · 2023-12-15T15:35:07Z

+    });
+  }
+
+  await pendingLogin;


Good idea. 👍

ravicious · 2023-12-18T14:12:28Z

+  if (!pendingLogin) {
+    pendingLogin = login(appContext, rootClusterUri).finally(() => {
+      pendingLogin = undefined;
+    });
+  }


I did some tests too and everything appears to be working fine. 👍

public-teleport-github-review-bot · 2023-12-18T14:33:47Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v12	Create PR
branch/v13	Create PR
branch/v14	Create PR

Support concurrent requests in retryWithRelogin

654b15e

gzdunek requested a review from ravicious December 11, 2023 11:51

gzdunek added backport/branch/v12 no-changelog Indicates that a PR does not require a changelog entry labels Dec 11, 2023

github-actions Bot requested review from rudream and ryanclark December 11, 2023 11:51

github-actions Bot added size/sm ui labels Dec 11, 2023

rudream approved these changes Dec 11, 2023

View reviewed changes

ryanclark approved these changes Dec 13, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from ravicious December 13, 2023 10:41

ravicious self-requested a review December 13, 2023 10:50

ravicious requested changes Dec 14, 2023

View reviewed changes

ravicious approved these changes Dec 15, 2023

View reviewed changes

gzdunek added 2 commits December 15, 2023 18:00

Cancel the previous dialog when opening a new one

be73abc

pendingLogin -> pendingLoginDialog

602a7c1

gzdunek requested a review from ravicious December 15, 2023 17:07

ravicious approved these changes Dec 18, 2023

View reviewed changes

gzdunek added this pull request to the merge queue Dec 18, 2023

Merged via the queue into master with commit 1b2f271 Dec 18, 2023

gzdunek deleted the gzdunek/concurrent-retry-with-relogin branch December 18, 2023 14:31

This was referenced Dec 18, 2023

[v14] Support concurrent requests in retryWithRelogin #35825

Merged

[v13] Support concurrent requests in retryWithRelogin #35826

Merged

[v12] Support concurrent requests in retryWithRelogin #35827

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support concurrent requests in `retryWithRelogin`#35599

Support concurrent requests in `retryWithRelogin`#35599
gzdunek merged 3 commits intomasterfrom
gzdunek/concurrent-retry-with-relogin

gzdunek commented Dec 11, 2023

Uh oh!

ravicious Dec 14, 2023

Uh oh!

gzdunek Dec 15, 2023 •

edited

Loading

Uh oh!

ravicious Dec 15, 2023

Uh oh!

gzdunek Dec 15, 2023

Uh oh!

ravicious Dec 18, 2023

Uh oh!

ravicious Dec 14, 2023

Uh oh!

gzdunek Dec 15, 2023

Uh oh!

ravicious Dec 15, 2023

Uh oh!

ravicious Dec 15, 2023

Uh oh!

ravicious Dec 15, 2023

Uh oh!

ravicious Dec 18, 2023

Uh oh!

public-teleport-github-review-bot Bot commented Dec 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

gzdunek commented Dec 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented Dec 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gzdunek Dec 15, 2023 •

edited

Loading