Abort the inflight fetch() when networkTimeoutSeconds is reached

[pavlexander]

Library Affected: "workbox-background-sync": "^6.5.3", "workbox-core": "^6.5.3", "workbox-precaching": "^6.5.3", "workbox-window": "^6.5.3"

Browser & Platform: all browsers

Issue or Feature Request Description: I have noticed 2 issues with handlerDidError callback.

Base code:

import { Queue } from "workbox-background-sync";
import { Strategy, NetworkOnly } from "workbox-strategies";

const queue = new Queue("myQueueName", {
  onSync: async ({ queue }) => {
    let entry;
    while ((entry = await queue.shiftRequest())) {
      try {
        var response = await fetch(entry.request.clone());
      } catch (error) {
        await queue.unshiftRequest(entry);
        throw error;
      }
    }
  },
});

const statusPlugin = {
  handlerWillRespond: async ({ request, response, event, state }) => {
    console.log("handlerWillRespond");
    return response;
  },
  fetchDidFail: async ({ originalRequest, request, error, event }) => {
    console.log("fetchDidFail");
    //console.log(await originalRequest.clone().json()); // works

    await queue.pushRequest({ request: originalRequest });
  },
  fetchDidSucceed: async ({ response }) => {
    console.log("fetchDidSucceed");
    //console.log(await response.clone().json()); // works

    return response;
  },
  handlerDidError: async ({ error, event, request, state }) => {
    console.log("handlerDidError");
    //console.log(await request.clone().json()); // TypeError: Failed to execute 'clone' on 'Request': Request body is already used
  },
};

registerRoute(
  new RegExp(".*/Exercises/.*"),
  new NetworkOnly({
    networkTimeoutSeconds: 3,
    plugins: [statusPlugin],
  }),
  "POST"
);

Issue 1: Unable to read the request data

Following code:

  handlerDidError: async ({ error, event, request, state }) => {
    console.log("handlerDidError");
    console.log(await request.clone().json()); //  TypeError: Failed to execute 'clone' on 'Request': Request body is already used
  },

will throw:

TypeError: Failed to execute 'clone' on 'Request': Request body is already used

Issue 2: handlerDidError runs before fetchDidFail (subjective)

Current callback flow is:

1. client makes a request
2. network is unavailable
3. handlerDidError is consoled
4. clients gets the response
5. fetchDidFail is consoled

I would expect all the pipelines related to errors to be executed before client sees the response.

Temporary solution

I have managed to fix both of the problems by using the custom Network strategy

class NewStrategy extends NetworkOnly {
  async _handle(request, handler) {
    return await handler.fetch(request.clone());
  }
}

See that I have cloned the request before passing it to handler. This will allow us to clone the request later in handlerDidError callback. The second "issue" is also solved. Now the flow is:

1. client makes a request
2. network is unavailable
3. fetchDidFail is consoled
4. handlerDidError is consoled
5. clients gets the response

See that the handlerDidError is actually run After the fetchDidFail. This makes sense because the handlerDidError is supposed to be the last logical thing in the pipeline: https://developer.chrome.com/docs/workbox/using-plugins/

handlerDidError: Called if the handler can't provide a valid response from from any source, which is the optimal time to provide some sort of fallback response as an alternative to failing outright. // Return response, a different Response object as a fallback, or null.

I might be wrong about the pipeline order thingy (it just looks more logical to me) but the "cloning" issue seems to be a valid one regardless.

[jeffposnick]

Generally speaking, fetchDidFail should fire before handlerDidError. You can see this happen at, e.g., https://sudden-futuristic-health.glitch.me/ where there's a request that fails because the domain name resolution is unsuccessful.

One exception, that you might be bumping up into, is when the overall strategy fails due to a timeout imposed by networkTimeoutSeconds, without a valid cached response to fall back on. In that scenario, I do see how handlerDidError would end up firing first, before either fetchDidSucceed or fetchDidFail (depending on whether the underlying request, which is make independently from the timeout, ended up succeeding or not). The cleanest solution there might be to use abortable fetch(), which is now widely supported, and actually terminate the associated fetch() when networkTimeoutSeconds is reached. At the same time, we should not trigger either fetchDidFail or fetchDidSucceed when that happens. Maybe there should be a fetchDidAbort event that gets triggered, though? We can leave this issue open to track that.

Regarding the other issue, I would recommend working around that by saving a clone of the outgoing request in the state parameter that's passed to the requestWillFetch callback, and then referencing it later on in, e.g., handlerDidError. You're guaranteed to get the same state value passed to all of the callbacks associated with the same fetch event.

const cloneRequestPlugin = {
  requestWillFetch: async ({event, request, state}) => {
    state.requestClone = request.clone();
    return request.
  },

  handlerDidError: async ({error, event, request, state}) => {
    // Do something with state.requestClone
  },
};

[pavlexander]

Generally speaking, fetchDidFail should fire before handlerDidError. You can see this happen at, e.g., https://sudden-futuristic-health.glitch.me/ where there's a request that fails because the domain name resolution is unsuccessful.

You are correct. I have just tested with and without networkTimeoutSeconds parameter and the fetchDidFail is only triggered before handlerDidError when no value is set.. I would be very careful with abortable fetch option, given that it hasn't been around for long enough, unless there is a way to make it be backwards compatible with non-abortable fetch.. Otherwise the suggestion is nice.

Regarding the other issue, I would recommend working around that by saving a clone of the outgoing request in the state parameter that's passed to the requestWillFetch callback, and then referencing it later on in, e.g., handlerDidError. You're guaranteed to get the same state value passed to all of the callbacks associated with the same fetch event.

It works! Now I know what the state parameter could/should be used for :) I guess it's a nice workaround.. So is this correct that the body of the request is not accessible in the handlerDidError callback? Personally I have no problems with that, given that we could use the state to pass the data around across the pipeline.

Thank you for your reply.

[jeffposnick]

I've filed https://github.com/GoogleChrome/developer.chrome.com/pull/2751 to move that context about using state to store a cloned request to our official docs.