Time out initial fetch, and go to account-picker screen #4165

gnprice · 2020-06-17T19:59:53Z

If you launch the app, and the last server you'd been using is one that is now impossible to reach... currently we just stay at the loading screen forever, endlessly retrying the initial fetch.

If that's the only server you ever wanted to use, then this isn't so bad, given that there's not much useful we can do until the server is back. But if you'd like to go use some other server, then you're completely blocked from doing so. If the server doesn't come back, your only remedy is to clear the app's data entirely (or uninstall and reinstall) and log into those other servers from scratch. This is #4159, as described at #4159 (comment) .

We should do better. As a minimal version, if the initial fetch takes a completely unreasonable amount of time (maybe one minute), we should give up and take you to the account-picker screen so you can try a different account and server.

(An improved followup version might continue loading but show a message like "Still working..." and an action "Switch account" on the loading screen, after a much shorter period like 7 seconds.)

Here's how I'd go about implementing this:

The key logic to change is in doInitialFetch, around api.registerForEvents with the pair of tryUntilSuccessful calls.
That logic already handles the case where the load is outright rejected by the server -- that's the isClientError case inside tryUntilSuccessful, and the catch block outside it that calls dispatch(logout()).
What's needed is to also handle the case where the server simply doesn't respond, or responds with a 5xx error. We don't want to log out in that case, because the credentials may still be perfectly good -- but we do want to abort.
For actually aborting and going to the account-picker screen, we can probably do the same thing as we do for the "Switch account" button in ProfileCard.js: dispatch(navigateToAccountPicker()). Then return out of doInitialFetch.
For the logic to get there: first, let's move tryUntilSuccessful into the same file as doInitialFetch and just above it, and give it a more specific name like tryFetch. (This move can be its own commit.) It's always been closely tied to that function's specific needs with its handling of 4xx errors, and we're about to make it more so.
Then, in the retry loop, check how long it's been since we started -- if it's more than a timeout constant like 60 seconds, throw an exception. In the catch block around the requests, still do dispatch(logout()) if isClientError(e); but otherwise, abort without logging out.

The text was updated successfully, but these errors were encountered:

chrisbobbe · 2020-06-17T21:20:14Z

For the logic to get there: first, let's move tryUntilSuccessful into the same file as doInitialFetch and just above it, and give it a more specific name like tryFetch. (This move can be its own commit.) It's always been closely tied to that function's specific needs with its handling of 4xx errors, and we're about to make it more so.

fetchPrivateMessages also uses tryUntilSuccessful. Does that look like a mistake, now that we're going to change tryUntilSuccessful to more specifically address doInitialFetch's needs? If it's not a mistake, I wonder why, e.g., fetchMessagesInNarrow doesn't use it.

gnprice · 2020-06-17T22:23:07Z

fetchPrivateMessages also uses tryUntilSuccessful. Does that look like a mistake, now that we're going to change tryUntilSuccessful to more specifically address doInitialFetch's needs?

Huh, good catch -- I'd looked for uses of tryUntilSuccessful but somehow I'd missed that one!

Reading it, I think it'd be fine for it to continue to use the helper when it grows a timeout, or also fine for it to just have its current behavior inlined as a BackoffMachine loop... or really basically fine for it to just try once and not bother retrying, just as fetchMessages does. (Especially as this function is doomed anyway, as the jsdoc tells us.)

I think the reason it differs from fetchMessagesInNarrow is just that this helper was factored out from the initial fetch (in 0a3149c), and it had the retry there because the rest of the initial fetch does.

`tryUntilSuccessful` is about to grow a timeout, which we've had in mind in particular for the initial fetch. It's also about to be moved into this file, and we want to keep its association with `doInitialFetch` by putting it directly above it, with nothing in between. Removing this call site lets us do so without ESLint complaining of `no-use-before-define`. And Greg says it's all the same to him [1], especially since `fetchPrivateMessages` is doomed to be deleted anyway, as the JSDoc says. [1]: zulip#4165 (comment)

As Greg says [1], it's always been closely tied to `doInitialFetch`'s specific needs, with its handling of 4xx errors. We're about to make it more so, by adding timeout logic. [1]: zulip#4165 (comment)

Using `INITIAL_FETCH_ABORT` and `promiseTimeout`, which we set up in the last few commits, abort and go to the accounts screen if the initial fetch is taking too long. Fixes: zulip#4165

As Greg says [1], it's always been closely tied to `doInitialFetch`'s specific needs, with its handling of 4xx errors. We're about to make it more so, by adding timeout logic. Also, remove two pretty trivial tests, and adapt the third (that tests that a retry is done) to use Lolex. Add another test to confirm that a 4xx error does indeed halt the retry loop and get thrown. [1]: zulip#4165 (comment)