Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time out initial fetch and go to account-picker screen. #4754

Merged
merged 13 commits into from
Jul 16, 2021

Commits on Jul 16, 2021

  1. async: Add promiseTimeout.

    We'll use this in `tryFetch`, in an upcoming commit, so that we can
    interrupt in-progress attempts to contact a server when they take
    too long. See discussion [1].
    
    [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Stuck.20on.20loading.20screen/near/907693
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    424d1c9 View commit details
    Browse the repository at this point in the history
  2. fetchActions: Improve jsdoc.

    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    2436ac2 View commit details
    Browse the repository at this point in the history
  3. redux: Add boilerplate for INITIAL_FETCH_ABORT action.

    To be dispatched when we want to give up on the initial fetch.
    
    It navigates to the 'accounts' screen so a user can try a different
    account and server. Logging out wouldn't be good; the credentials
    may be perfectly fine, and we'd like to keep them around to try
    again later.
    
    It sets `needsInitialFetch` to `false` [1], just like
    `INITIAL_FETCH_COMPLETE`, while retaining a different meaning than
    that action (i.e., that the fetch was aborted instead of completed).
    
    Setting `needsInitialFetch` to false is necessary to ensure that a
    subsequent initial fetch can be triggered when we want it to be. As
    also noted in 7caa4d0, `needsInitialFetch` is "edge-triggered".
    (That edge-triggering logic seems complex and fragile, and it would
    be nice to fix that.)
    
    See also discussion at
      https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Stuck.20on.20loading.20screen/near/907591.
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    a19108c View commit details
    Browse the repository at this point in the history
  4. fetchActions: Fail early on all non–known-retryable errors in `tryFet…

    …ch`.
    
    The TODO doesn't specifically mention that we want to retry on
    network-failure errors, but we do, so add that too.
    
    If we get some strange, totally unexpected error caused by a bug
    somewhere, we shouldn't continue the retry loop. Not only would that
    waste the user's time, but it means the error basically gets
    swallowed, and it makes it much harder for us to detect it and debug
    effectively. So, fail early on exceptions like that.
    
    See our doc on exception handling:
      https://github.com/zulip/zulip-mobile/blob/master/docs/style.md#catch-at-ui.
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    94b7444 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    bf28c95 View commit details
    Browse the repository at this point in the history
  6. fetchActions: Add a timeout to tryFetch.

    So far, `tryFetch`'s only callers are in the initial fetch; so, add
    handling for the `TimeoutError` there.
    
    The choice of value for `requestLongTimeoutMs` comes from a
    suggestion in zulip#4165's description:
    
    > 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.
    
    Fixes: zulip#4165
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    93ec9d8 View commit details
    Browse the repository at this point in the history
  7. fetchActions: Also abort on app-internal errors in the initial fetch.

    As Greg points out [1]:
    
    > Basically it's an internal error in the app. But the user is
    > probably better off getting kicked back to the account picker than
    > having the loading screen continue indefinitely -- it means at
    > least they can try again and see if the issue recurs, or try
    > another account and see if they can at least use that.
    
    [1] zulip#4754 (comment)
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    2db5f9d View commit details
    Browse the repository at this point in the history
  8. fetchActions: Pull await backoffMachine.wait() out of catch block.

    As Greg points out [1], this makes the most sense conceptually; it's
    happening at the bottom of the loop, just before a new iteration
    starts. The `return` in the `try` block is enough to ensure that
    this wait won't interfere with a successful fetch.
    
    [1]: zulip#4166 (comment)
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    e76252d View commit details
    Browse the repository at this point in the history
  9. Configuration menu
    Copy the full SHA
    7664460 View commit details
    Browse the repository at this point in the history
  10. fetchActions: Don't always retry the initial fetch on server errors.

    Greg points out that we don't always want to retry the initial fetch
    on server errors [1]:
    
    > Ah, I think in #M4165 the point is that if the server isn't
    > responding, we want to give you the option to go choose some other
    > account. The context there is that we're in the initial fetch, so
    > showing the loading screen, and as long as we're doing that
    > there's no other UI.
    
    > So yeah, I think basically we don't want to do any retrying here.
    > Instead we can kick you to the account-picker screen, with a toast
    > or something to indicate an error, and then you might manually
    > retry a time or two or you might bail and switch to some other
    > account.
    
    > And in particular if you didn't even want to be using that account
    > anymore -- maybe you even know that it's a server which is
    > permanently shut down, but it just happened to be the last one
    > you'd been using in the app and so it's the one we tried loading
    > data from on startup -- then you can go use whatever other account
    > you were actually opening the app to use.
    
    If we're in an initial fetch from an already active, logged-in
    account that we have cached data for, though, we might as well take
    the time to retry (still with the 60 second timeout) because the
    user will be seeing a useful, interactive UI.
    
    [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Stuck.20on.20loading.20screen/near/1178689
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    7ed53a2 View commit details
    Browse the repository at this point in the history
  11. fetchActions: Retry message fetch with a timeout.

    A `TimeoutError` will be handled the same way other errors in
    `fetchMessages` are handled; if it's a timeout in the fetch
    `ChatScreen` does on mount, `ChatScreen` will show the `FetchError`
    component we set up in zulip#4205.
    
    There's also been a passing mention on CZO of doing a timeout like
    this [1]:
    
    > After a long time, probably like a minute, we'll want that [...]
    > fetch to time out and fail in any case.
    
    [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/950853
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    3ca092f View commit details
    Browse the repository at this point in the history
  12. FetchError: Show a specific message if the error is a TimeoutError.

    And add that message and the existing message to messages_en.json;
    looks like we forgot to add the existing one.
    chrisbobbe authored and gnprice committed Jul 16, 2021
    Configuration menu
    Copy the full SHA
    06f07c0 View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    ab62f73 View commit details
    Browse the repository at this point in the history