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

What happens to the request body if the fetch isn't handled? #1191

Open
jakearchibald opened this issue Aug 30, 2017 · 14 comments
Open

What happens to the request body if the fetch isn't handled? #1191

jakearchibald opened this issue Aug 30, 2017 · 14 comments

Comments

@jakearchibald
Copy link
Contributor

What happens if:

  • POST request with body.
  • The service worker does not call respondWith.
  • After 100ms (using waitUntil) the service worker tries to read the body of the request.

It feels like the read should fail, since the body is used as part of making the default request.

Test – Both Firefox and Chrome allow the body to be read, as if it's a clone.

I'm not sure this behaviour is compatible with request body streams. Or, it isn't memory-friendly.

Also, what happens if:

  • POST request with body.
  • The service worker gets a lock on the request's stream, tries to read data.
  • The service worker does not call respondWith.

It feels like this should result in a network error, as the browser won't be able to read from the stream.

Test – Both Firefox and Chrome allow the body to be read in the service worker, and allow the default request to happen.

Again, I think this is problematic with request body streams.

Shall we try and change this?

@annevk @wanderview

@wanderview
Copy link
Member

Actually, doesn't it kind of have to be a clone? The original Request may live on the main thread while the FetchEvent.request lives on the worker thread. Any Request.body is going to have to have been transferred across the thread boundary.

@annevk
Copy link
Member

annevk commented Aug 30, 2017

Well the UA could only transfer it at the point the service worker requests it. For such a UA deciding that there's only a small window it can be requested can be beneficial. Not sure what that means for the second case though. Though even there it could be beneficial if you can share the buffers across threads and not have to copy them.

@wanderview
Copy link
Member

Well the UA could only transfer it at the point the service worker requests it.

How is that possible? Request.body is synchronous.

In general I would expect a main thread fetch() to mark any Request.body disturbed immediately. It would then read it at some rate based on consumption. If a SW intercepts and is slow to consume, etc, then it may not be read quickly. In theory it might be partially read and then closed if the SW decides not to consume it at all.

@jakearchibald
Copy link
Contributor Author

From a spec point of view, I think this is pretty easy. If the user doesn't call respondWith, we disturb request.body. Over in the fetch spec, once "handle fetch" is called, if no response is provided and request.body is disturbed, return a network error.

@jakearchibald
Copy link
Contributor Author

In general I would expect a main thread fetch() to mark any Request.body disturbed immediately.

I believe it does.

@wanderview
Copy link
Member

From a spec point of view, I think this is pretty easy. If the user doesn't call respondWith, we disturb request.body. Over in the fetch spec, once "handle fetch" is called, if no response is provided and request.body is disturbed, return a network error.

That works for me.

@annevk
Copy link
Member

annevk commented Aug 30, 2017

@wanderview but the developer of the service worker needs to take some kind of action to actually get the bytes. It seems there are optimization opportunities around that in theory. The UA could not start cloning until the bytes are accessed, the UA could clone them partially and then discontinue the operation if respondWidth() is not invoked quickly enough, etc.

@wanderview
Copy link
Member

Sure, but in practice the best optimization for streams tends to be "read eagerly up to some buffering limit, then apply back pressure". If we didn't buffer at all until the first read it would be a performance penalty.

@jakearchibald
Copy link
Contributor Author

F2F: #1191 (comment) is fine. If event.respondWith(Response.redirect(…)) indicates that the body should be resent, it should network error if the body has been locked/disturbed.

@annevk
Copy link
Member

annevk commented Nov 8, 2017

What does that mean in practice? If a POST a string "x" and the service worker redirects, would that result in a network error? (Upthread it's stated that the main thread disturbs all bodies.) That seems like bad behavior in a way as it means that a service worker yields different results from a server.

See also whatwg/fetch#538 where I said that same thing to @mattto. Was that point discussed during the F2F?

@mfalken
Copy link
Member

mfalken commented Nov 20, 2017

If I remember correctly at the F2F we were happy with making the service worker use clone() to avoid breakage, but maybe that should be reconsidered. It does seem a bit weird for a 307 redirect to automatically break even if the worker didn't read the request.body.

I put together a test page here: https://aware-flame.glitch.me/

The current Chrome behavior is liable to change as we migrate to the the "servicified" architecture, which is trying to avoid needless data copies (right now we always stuff the whole request body in a new blob and hand that to the service worker, so the body survives whatever the service worker does).

In the "read from sw and then redirect 307" fetch() test, Firefox seems to get a Bad Request response but I can't determine whether that's coming from the server for some reason or from Firefox. Based on the discussion here, it should be a network error. However, for main resource (navigation) requests, the body is available after the redirect even though service worker consumed it.

@mfalken
Copy link
Member

mfalken commented Jun 5, 2019

I tried looking over browser behavior and it seems they mostly allow reading from both the service worker and server (i.e., it doesn't get disturbed explicitly).

Here's intuitively what I think is the expected behavior:

  • If the service worker doesn't touch request.body and does not call respondWith() , the request falls back with an intact body.
  • If the service worker doesn't touch request.body and responds with Response.redirect(), the redirected request has the intact body.
  • If the service worker touches request.body, it's disturbed and network fallback or redirect will result in a network error.
  • Once dispatching the fetch event finishes, request.body inside the service worker is disturbed and touching it throws an error.

This might contradict #1191 (comment) though, and we might need to add some state to Request objects to disturb the body while the underlying request's body is not yet closed.

cc @yutakahirano who might have thoughts.

@wanderview
Copy link
Member

I tried looking over browser behavior and it seems they mostly allow reading from both the service worker and server (i.e., it doesn't get disturbed explicitly).

I wonder if we could collect some data to determine how often this happens in practice. Because if its something that has become commonplace then we may not be able to easily do:

If the service worker touches request.body, it's disturbed and network fallback or redirect will result in a network error.

@mfalken
Copy link
Member

mfalken commented Jun 6, 2019

That's fair, I'll try to do that. I was originally motivated to close the hole so we don't get stuck supporting it forever on accident but it might already be past that point.

I'll look more closely how this works in Chrome. I recall not really doing this on purpose and I wouldn't be surprised if the behavior can break if something gets garbage collected/dropped while reading. On the other hand, to support 307 we have to keep the data source around somewhat anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants
@jakearchibald @wanderview @annevk @mfalken and others