Add HTTP fetch step on 421.

[yoichio]

• At least two implementers are interested (and none opposed):
  • Chrome
  • Firefox
• Tests are written and can be reviewed and commented upon at:
  • Add fetch upload test on HTTP 421 response. web-platform-tests/wpt#27322
• Implementation bugs are filed:
  Chrome: As-is for non-streaming request. https://crbug.com/1171011 for streaming request.
  Firefox: As-is for non-streaming request. N/A as streaming request body has not been implemented
  Safari: https://bugs.webkit.org/show_bug.cgi?id=222789 for non-streaming request. N/A as streaming request body has not been implemented

(See WHATWG Working Mode: Changes for more details.)

Fixes #497, #982

---

Preview | Diff

[annevk]

Do we also want to do this if the service worker returns a 421 or only if the network returns one? We should probably have test coverage either way.

Also, I think we should cover what should happen for a non-streaming request.

Could you also restore the pull request template?

[yoichio]

Do we also want to do this if the service worker returns a 421 or only if the network returns one?

Both Chrome and Firefox retry only if the network returns 421.

Also, I think we should cover what should happen for a non-streaming request.

Chrome does this retry during "7.3. Set response to the result of making an HTTP request over connection using request" in 4.6. HTTP-network fetch.
How might we write it down ?

We should probably have test coverage either way.

Sure.

Could you also restore the pull request template?

Done.

[annevk]

In https://fetch.spec.whatwg.org/#http-network-or-cache-fetch we have checks for 401 and 407. I think we should handle 421 there as well, in a similar manner. This means that if a service worker creates a 421 the API will see it. Is there any kind of limit on the number of retries?

[yoichio]

Though fetch event is captured by a service worker or not, HTTP-network fetch retries only once.

[yutakahirano]

The PR description is copied from elsewhere. Please update it.

[yoichio]

The PR description is copied from elsewhere. Please update it.

What kind of update is needed?

[yoichio]

Updated #http-network-or-cache-fetch after 407 checking. PTAL.
I'm not sure how we can limit the retry only once.

[yutakahirano]

https://crbug.com/1167013 is a different bug.

"N/A as streaming request body has not been implemented" is incorrect for this change because "we should cover what should happen for a non-streaming request."

[yoichio]

Updated the title and the description:

• Add wpt pull-request
• Update Implementation bugs

[yutakahirano]

I think we should have a dedicated issue for Chromium (for streaming request).

[yoichio]

Done (https://crbug.com/1171011: Fetch streaming upload should fail on a 421 (Misdirected Request) response).

[yutakahirano]

The current logic leads to endless retry. Using another connection, which is required by the RFC, is also missing.

I'm not sure how in detail it should be specified, but at least some notes are needed. @annevk, thoughts?

[annevk]

I think it's okay for the initial version to have something like

User agents should have an <a>implementation-defined</a> recursion limit.

as a separate paragraph below the recursive invocation. It's not immediately clear to me if this limit can be exploited in some way, but I guess we'll learn that over time...

[annevk]

This will also fix #497 and we should note that in the final commit message.

[yoichio]

Added recursion limit and updated the PR description including #497 .

[annevk]

Indentation could use some cleanup, but otherwise this looks good to me.

[annevk]

This looks good to me, except for the one missing parameter. I have some editorial nits I can push in a fixup commit.

[yoichio]

I misunderstood column was in 80 characters instead of 100.
Thanks. LGTM.

[annevk]

@yoichio I updated this per the discussion in httpwg/http-core#789. In particular:

1. If you get a 421 while you do a stream upload, we just pass the 421 on to the caller.
2. If you get a 421 a second time, we just pass the 421 on the caller (and don't allow for another retry).

Are you willing to update the tests if that seems reasonable? I can also help out. Just let me know.

[yoichio]

LGTM. I'll update the test.

[annevk]

I rebased this again. So "obtain a connection" now has bypassConnectionPool and dedicated. bypassConnectionPool bypasses the connection pool on lookup, but still stores the result in the connection pool, and dedicated bypasses it completely. Should we organize this differently?

@martinthomson who had naming suggestions on the WebTransport PR might have insights here as well.

[annevk]

One thought I had was to rename this parameter to forceNew to make it clearer it only forces a new connection, nothing else.

[yoichio]

forceNew sounds good to me.

[annevk]

Thanks @yoichio and @yutakahirano. I'll merge this now and I'll file a bug against Safari so they can consider supporting 421 responses.