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

[chrisbobbe]

Fixes: #4165

N.B.: Extracting promiseTimeout into a helper function, as @gnprice suggested here, led me to add some time-elapsed arithmetic near the call site so we're not too generous and give each attempt a full 60 seconds. We shrink timeLimitMs with each successive call.

[chrisbobbe]

Oh, goodness—I do need to add tests for promiseTimeout, I think. And probably tryFetch; I removed the tests for that under its previous name, tryUntilSuccessful, partly because they weren't really doing much. Glad to see that there are already tests for isClientError.

Does that sound right?

[gnprice]

Yeah, let's have tests for promiseTimeout. 🙂 Then for tryFetch, two of the three old tests were pretty trivial, but the third tests that it does retry. It'd also be good to test its new timeout functionality, and the existing isClientError feature.

[gnprice]

Thanks @chrisbobbe ! This looks like a good solution. Some comments below.

[gnprice]

I like this term "outcome". It's a good way of conveying this should apply to completion and failure uniformly.

[gnprice]

This feels like ideally it belongs at the same place as the new TimeoutError(), and replacing that in the onTimeout case. I think that would simplify the code, because this whole try/catch would then get to go away.

IOW, the whole try/catch could become just

  return await Promise.race([promise, timeoutPromise]);

with a suitable version of timeoutPromise that itself handles the onTimeout.

(And then in fact the await could be deleted, along with the async: just return Promise.race(…);.)

[chrisbobbe]

Ah, I thought of doing it that way—but then, IIUC, it's not a race between (A) the outcome of promise and (B) the timer (which is what it says on the tin), but rather, between (A) the outcome of promise and (B) the timer plus the outcome of a Promise returned by onTimeout.

[chrisbobbe]

Mm, bumping this, @gnprice, as I don't think we got around to it on the call (we were mostly talking about #4166 (comment)).

[chrisbobbe]

Concretely, I think you're suggesting something like this:

  const timeoutPromise = new Promise((resolve, reject) =>
    setTimeout(
      () => (onTimeout !== undefined ? resolve(onTimeout()) : reject(new TimeoutError())),
      timeLimitMs,
    ),
  );
  return Promise.race([promise, timeoutPromise]);

[chrisbobbe]

But this gives promise too much time to complete; it now has timeLimitMs plus however long a Promise returned by onTimeout takes. Worse, if this gives enough time for promise to complete, then we're in a weird in-between state where onTimeout has fired (i.e., the time is properly up) but the outcome is the outcome of promise.

[chrisbobbe]

resolve(onTimeout()) might look a bit weird if onTimeout returns a Promise; I'm not used to doing that. But I think this excerpt supports the idea that the outcome of onTimeout() (the Promise returned by onTimeout—which may take arbitrarily long to arrive) is assigned to be the outcome of timeoutPromise.

It's from a big green note here, in a section on the Promise constructor in an ECMAScript spec (formatting lost):

The resolve function that is passed to an executor function accepts a single argument. The executor code may eventually call the resolve function to indicate that it wishes to resolve the associated Promise object. The argument passed to the resolve function represents the eventual value of the deferred action and can be either the actual fulfillment value or another Promise object which will provide the value if it is fulfilled.

[chrisbobbe]

I'll include a test for this case and you can play around with it if you have doubts. 🙂

[gnprice]

Hmm, good point. And good thought to add a test for it. 🙂

It still feels like this logic is sort of awkwardly split between the new TimeoutError() and the later catch and if (e instanceof TimeoutError && onTimeout !== undefined). But evidently it'd take some deeper thinking about these promise and timer semantics to fix that than I'd thought. And there isn't that much to gain -- the behavior seems correct as it is, and the code is already short and not hard to understand.

[gnprice]

Ideally this would also be covered by the logic expressed by timeLimitMs above.

[chrisbobbe]

Hmm, true—see one possible way this might look, at #4166 (comment).

[gnprice]

Looking at this and particularly with the comment I just made about the backoffMachine.wait(), I wonder: with the spiffy new promiseTimeout helper, could all this timeout logic be simplified by using that around the whole thing? I think it could.

[chrisbobbe]

Hmm, if I understand correctly, something like this?

export async function tryFetch<T>(func: () => Promise<T>): Promise<T> {
  const MAX_TIME_MS: number = 60000;
  const backoffMachine = new BackoffMachine();

  return promiseTimeout<T>(
    (async () => {
      // eslint-disable-next-line no-constant-condition
      while (true) {
        try {
          return await func();
        } catch (e) {
          if (isClientError(e) || e instanceof TimeoutError) {
            throw e;
          }
          await backoffMachine.wait();
        }
      }
      // Without this, Flow 0.92.1 does not know this code is unreachable,
      // and it incorrectly thinks Promise<undefined> could be returned,
      // which is inconsistent with the stated Promise<T> return type.
      // eslint-disable-next-line no-unreachable
      throw new Error();
    })(),
    MAX_TIME_MS,
  );
}

[chrisbobbe]

Oh, hmm, that's not quite it—the infinite loop could continue infinitely, while being ignored, even after the timer was up. I'll see what I can think of.

[chrisbobbe]

Er, there's probably a more elegant solution, but this might be working code:

export async function tryFetch<T>(func: () => Promise<T>): Promise<T> {
  const MAX_TIME_MS: number = 60000;
  const backoffMachine = new BackoffMachine();

  let timerHasExpired = false;

  return promiseTimeout(
    (async () => {
      // eslint-disable-next-line no-constant-condition
      while (true) {
        if (timerHasExpired) {
          // No one is listening for this Promise's outcome, so stop
          // doing more work.
          throw new Error();
        }
        try {
          return await func();
        } catch (e) {
          if (isClientError(e)) {
            throw e;
          }
          await backoffMachine.wait();
        }
      }
      // Without this, Flow 0.92.1 does not know this code is unreachable,
      // and it incorrectly thinks Promise<undefined> could be returned,
      // which is inconsistent with the stated Promise<T> return type.
      // eslint-disable-next-line no-unreachable
      throw new Error();
    })(),
    MAX_TIME_MS,
    () => {
      timerHasExpired = true;
      throw new TimeoutError();
    }
  );
}

[chrisbobbe]

It'd also be good to test its new timeout functionality, and the existing isClientError feature.

It looks like there are already tests for isClientError; did you have more thoroughness in mind?

I just pushed my latest revision, so this is ready for another review.

[gnprice]

Thanks for the revisions! Comments below.

[gnprice]

Clearer as sleep, I think: await sleep(10);.

(Ditto below.)

[gnprice]

Staring at this a bit to unpack it, I think it means the same thing as

const quickPromise = sleep(1).then(() => x);

Does that seem right? I think that would be easier to understand.

[gnprice]

And this would be sleep(1).then(() => { throw new Error(x.toString()); }).

[gnprice]

Can we use runAllTimers or runAllTimersAsync for all of these?

In general that seems easiest to think about -- it means we're running the whole thing to completion, and when we inspect the results at the end of the test there's no possibility we're seeing an intermediate state where there's still something more the code would do later if we let it.

There are cases where that isn't possible -- basically where there's an infinite loop -- but it seems worth reserving the greater care of thinking about intermediate states for those cases.

[gnprice]

It looks like in this version you don't end up using runOnlyPendingTimersAsync.

I suspect the key thing that was needed here wasn't the "async", but the "all". In the tests that use runAllTimersAsync, there's an onTimeout callback which... as its name predicts, gets called only after a timeout, and which then creates a new timeout. So runOnlyPendingTimers won't run that new timeout, because it wasn't already pending when runOnlyPendingTimers was invoked.

This is probably a sign that we should indeed make a habit of just using the "all" versions all the time 🙂, as I suggested in another comment just now, outside of the specific cases where there's a reason we can't.

[gnprice]

This can be pulled out to the top of the describe -- that'll make most of these tests a couple of lines shorter.

They're all doing very similar things, so the more we can shorten the repeating parts and highlight the parts that actually differ, the easier it is to see what-all is being tested.

[gnprice]

We have this pattern a lot in our Lolex-using tests, of:

• create a promise
• run timers
• await the promise.

The usual idiom is to combine "create promise" and "await promise" on the same line without giving the promise a name; but that isn't possible here because the "run timers" step needs to go in between.

How about pulling this pattern out as a function, perhaps named like withAllTimers? So e.g.

const result = await withAllTimers(promiseTimeout(endlessPromise, ONE_MINUTE_MS, onTimeout));

I think that'd help simplify a lot of these tests, and further contribute to isolating the parts that are interesting and different between them.

[gnprice]

This part is redundant with Flow, I think -- the implementation of promiseTimeout wouldn't pass the type-checker if it tried to pass arguments to a callback of type () => T | Promise<T>.

[chrisbobbe]

Closing, as this revision has grown old and doesn't have working tests—#4193 is working on that.