Tee request body on ServiceWorker interception #1144

yutakahirano · 2021-01-22T05:24:23Z

At least two implementers are interested (and none opposed):
- Chrome
- Firefox
Tests are written and can be reviewed and commented upon at:
- Test cases where request body gets used before network fallback web-platform-tests/wpt#27325
Implementation bugs are filed:
- Chrome: https://crbug.com/1167013
- Firefox: N/A as streaming request body has not been implemented
- Safari: N/A as streaming request body has not been implemented

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

Fixes w3c/ServiceWorker#1560, fixes #572.

yutakahirano · 2021-01-22T05:30:04Z

annevk · 2021-01-22T10:29:11Z

So what's the idea for non-streaming bodies that end up consumed by the service worker? It seems you decided to not clone the request for those scenarios?

yutakahirano · 2021-01-22T11:42:56Z

Sorry you're right. The latest PR includes an explicit copy step.

annevk

This looks good I think, but unlike https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch we do not copy request here. Is request's body the only thing the service worker can affect? Is the headers list immutable? (I'm not saying this needs to change, but I'd like to ensure we have the right setup here.)

I'd also appreciate review from @wanderview and @jakearchibald.

yutakahirano · 2021-01-27T08:29:58Z

This looks good I think, but unlike https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch we do not copy request here. Is request's body the only thing the service worker can affect? Is the headers list immutable? (I'm not saying this needs to change, but I'd like to ensure we have the right setup here.)

The latest PR creates a copy of a request.

annevk · 2021-01-28T11:25:28Z

Could you explain why you make a copy?

yutakahirano · 2021-01-28T11:29:46Z

To make it clear that request mutation in service worker doesn't affect the network network.

annevk · 2021-01-28T11:34:44Z

Yeah I was wondering if that was possible, but looking at https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm Headers should be immutable so I don't think we need it.

yutakahirano · 2021-01-28T12:23:43Z

Reverted the change partially.

jakearchibald

LGTM, but it isn't clear to me why the body is copied

jakearchibald · 2021-01-28T15:37:01Z

fetch.bs

+   <a for=/>body</a>.
+
+   <li>
+    <p>Let <var>savedBody</var> be a copy of <var>request</var>'s <a for=request>body</a>.


What's the benefit of copying the body at this point?

When calling clone() in the service worker it modifies reuqests.body. So we want to keep the original body.

Makes sense! Might be worth putting that in a note?

I'm not sure I understand. If you clone a request it uses https://fetch.spec.whatwg.org/#concept-body-clone to clone its body, right? This updates body's stream to a non-null value but its source remains the same. Couldn't we always extract from source again?

The point I'm trying to make is that it overwrites the request's body's stream and that means request will not be disturbed nor locked. I'd like to fail the request in such a case too (if other conditions are met). This corresponds to "clone-and-ignore" tests in web-platform-tests/wpt#27325.

If we tee the request body before passing it to the service worker (it is hard to implement TBH - I think we will not be able to implement it in the first release), I would like to allow reading the body in the network fallback case. I'm uncomfortable with having magic in the clone operation, and that is more important to me than enforcing the "read once" rule. @annevk, what do you think?

Can't we check if request's body's source is null and request's body's stream is disturbed or locked before we want to return null?

Why do yo prefer global "read once" rule to a stream body?

To give web developers an API that's as consistent as possible and also give implementations freedom to experiment with various memory optimization techniques.

OK, now it clones the original request. I'm going to fix w3c/ServiceWorker#1563.

annevk · 2021-02-16T11:45:30Z

@yutakahirano I don't see the closing of these streams as @jakearchibald suggested. Don't we need to do that? I guess it's not strictly speaking observable, but it seems somewhat bad to have a leak.

I like how this ended up being a lot simpler than some of the earlier approaches.

yutakahirano · 2021-02-16T12:00:48Z

I added a cancel step at in w3c/ServiceWorker#1563.

annevk · 2021-02-16T12:27:00Z

Shouldn't we close request here as well if we got a response back? We might also want to explain in a note what the setup is here.

yutakahirano · 2021-02-16T12:51:29Z

Shouldn't we close request here as well if we got a response back?

That is #572, I think (and yes, I think we should cancel it instead of transmitting).

annevk · 2021-02-16T12:54:21Z

Yeah, I'm thinking that we should fix #572 as you suggested after all. In that only the network layer attempts to transmit chunks.

…sable (#1563) Part of whatwg/fetch#1144

yutakahirano · 2021-02-18T14:52:39Z

Thank you!

Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <[email protected]> Co-authored-by: Anne van Kesteren <[email protected]>

This partially implements whatwg/fetch#1144 and w3c/ServiceWorker#1560. ServiceWorkerSubresourceLoader reuses the request body in the original request in the network fallback case (ServiceWorkerSubresourceLoader::OnFallback). This is problematic for a request with streaming upload body, because the body is not copyable. Ideally we should tee the body as specified in whatwg/fetch#1144 and w3c/ServiceWorker#1560, but this CL stops passing the body to the service worker instead to unblock the origin trial, assuming that few users care about the streaming body in the service worker. Bug: 1165690 Change-Id: Ie1d2d2fd74990b1bf7f9b10e55710a644871cc60

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <[email protected]> Co-authored-by: Anne van Kesteren <[email protected]> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <yhiranochrmomium.org> Co-authored-by: Anne van Kesteren <annevkannevk.nl> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325 UltraBlame original commit: fc2a98935821f5df9bf73b8642ff68ab95e1ea17

This partially implements whatwg/fetch#1144 and w3c/ServiceWorker#1560. ServiceWorkerSubresourceLoader reuses the request body in the original request in the network fallback case (ServiceWorkerSubresourceLoader::OnFallback). This is problematic for a request with streaming upload body, because the body is not copyable. Ideally we should tee the body as specified in whatwg/fetch#1144 and w3c/ServiceWorker#1560, but this CL stops passing the body to the service worker instead to unblock the origin trial, assuming that few users care about the streaming body in the service worker. Bug: 1165690 Change-Id: Ie1d2d2fd74990b1bf7f9b10e55710a644871cc60

This partially implements whatwg/fetch#1144 and w3c/ServiceWorker#1560. ServiceWorkerSubresourceLoader reuses the request body in the original request in the network fallback case (ServiceWorkerSubresourceLoader::OnFallback). This is problematic for a request with streaming upload body, because the body is not copyable. Ideally we should tee the body as specified in whatwg/fetch#1144 and w3c/ServiceWorker#1560, but this CL stops passing the body to the service worker instead to unblock the origin trial, assuming that few users care about the streaming body in the service worker. Bug: 1165690 Change-Id: Ie1d2d2fd74990b1bf7f9b10e55710a644871cc60 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2712842 Reviewed-by: Matt Falkenhagen <[email protected]> Reviewed-by: Yoichi Osato <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Auto-Submit: Yutaka Hirano <[email protected]> Commit-Queue: Yutaka Hirano <[email protected]> Cr-Commit-Position: refs/heads/master@{#857605}

This partially implements whatwg/fetch#1144 and w3c/ServiceWorker#1560. ServiceWorkerSubresourceLoader reuses the request body in the original request in the network fallback case (ServiceWorkerSubresourceLoader::OnFallback). This is problematic for a request with streaming upload body, because the body is not copyable. Ideally we should tee the body as specified in whatwg/fetch#1144 and w3c/ServiceWorker#1560, but this CL stops passing the body to the service worker instead to unblock the origin trial, assuming that few users care about the streaming body in the service worker. Bug: 1165690 Change-Id: Ie1d2d2fd74990b1bf7f9b10e55710a644871cc60 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2712842 Reviewed-by: Matt Falkenhagen <[email protected]> Reviewed-by: Yoichi Osato <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Auto-Submit: Yutaka Hirano <[email protected]> Commit-Queue: Yutaka Hirano <[email protected]> Cr-Commit-Position: refs/heads/master@{#857605}

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <[email protected]> Co-authored-by: Anne van Kesteren <[email protected]> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <yhiranochrmomium.org> Co-authored-by: Anne van Kesteren <annevkannevk.nl> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325 UltraBlame original commit: 2472b38eb2f886421d5a062314a87dc33029a393

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <yhiranochrmomium.org> Co-authored-by: Anne van Kesteren <annevkannevk.nl> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325 UltraBlame original commit: 02b382fb279e8edb2435787d95fd519f036320b3

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <yhiranochrmomium.org> Co-authored-by: Anne van Kesteren <annevkannevk.nl> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325 UltraBlame original commit: 2472b38eb2f886421d5a062314a87dc33029a393

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <yhiranochrmomium.org> Co-authored-by: Anne van Kesteren <annevkannevk.nl> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325 UltraBlame original commit: 02b382fb279e8edb2435787d95fd519f036320b3

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <yhiranochrmomium.org> Co-authored-by: Anne van Kesteren <annevkannevk.nl> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325 UltraBlame original commit: 2472b38eb2f886421d5a062314a87dc33029a393

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <yhiranochrmomium.org> Co-authored-by: Anne van Kesteren <annevkannevk.nl> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325 UltraBlame original commit: 02b382fb279e8edb2435787d95fd519f036320b3

…fallback on service worker, a=testonly Automatic update from web-platform-tests Make streaming upload work with network fallback on service worker This partially implements whatwg/fetch#1144 and w3c/ServiceWorker#1560. ServiceWorkerSubresourceLoader reuses the request body in the original request in the network fallback case (ServiceWorkerSubresourceLoader::OnFallback). This is problematic for a request with streaming upload body, because the body is not copyable. Ideally we should tee the body as specified in whatwg/fetch#1144 and w3c/ServiceWorker#1560, but this CL stops passing the body to the service worker instead to unblock the origin trial, assuming that few users care about the streaming body in the service worker. Bug: 1165690 Change-Id: Ie1d2d2fd74990b1bf7f9b10e55710a644871cc60 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2712842 Reviewed-by: Matt Falkenhagen <[email protected]> Reviewed-by: Yoichi Osato <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Auto-Submit: Yutaka Hirano <[email protected]> Commit-Queue: Yutaka Hirano <[email protected]> Cr-Commit-Position: refs/heads/master@{#857605} -- wpt-commits: ecb2781e1fde45e5ed70019f5ac9e14dfec065d1 wpt-pr: 27756

…fallback on service worker, a=testonly Automatic update from web-platform-tests Make streaming upload work with network fallback on service worker This partially implements whatwg/fetch#1144 and w3c/ServiceWorker#1560. ServiceWorkerSubresourceLoader reuses the request body in the original request in the network fallback case (ServiceWorkerSubresourceLoader::OnFallback). This is problematic for a request with streaming upload body, because the body is not copyable. Ideally we should tee the body as specified in whatwg/fetch#1144 and w3c/ServiceWorker#1560, but this CL stops passing the body to the service worker instead to unblock the origin trial, assuming that few users care about the streaming body in the service worker. Bug: 1165690 Change-Id: Ie1d2d2fd74990b1bf7f9b10e55710a644871cc60 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2712842 Reviewed-by: Matt Falkenhagen <falkenchromium.org> Reviewed-by: Yoichi Osato <yoichiochromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Auto-Submit: Yutaka Hirano <yhiranochromium.org> Commit-Queue: Yutaka Hirano <yhiranochromium.org> Cr-Commit-Position: refs/heads/master{#857605} -- wpt-commits: ecb2781e1fde45e5ed70019f5ac9e14dfec065d1 wpt-pr: 27756 UltraBlame original commit: 4bd27f41c457d02f5cd92bc5fae91f98f8149b3d

…before network fallback, a=testonly Automatic update from web-platform-tests Test cases where request body gets used before network fallback (#27325) Tests for whatwg/fetch#1144 and w3c/ServiceWorker#1563. Co-authored-by: Yutaka Hirano <[email protected]> Co-authored-by: Anne van Kesteren <[email protected]> -- wpt-commits: c30545df6a50c61f3673ed952c78c0a607100d45 wpt-pr: 27325

yutakahirano changed the title ~~Fail request when streaming request body is used in ServiceWorker~~ Fail request when its streaming body is used in ServiceWorker Jan 22, 2021

annevk added the topic: streams label Jan 22, 2021

annevk reviewed Jan 22, 2021

View reviewed changes

annevk requested review from jakearchibald and wanderview January 22, 2021 12:48

yutakahirano mentioned this pull request Jan 26, 2021

Test cases where request body gets used before network fallback web-platform-tests/wpt#27325

Merged

jakearchibald approved these changes Jan 28, 2021

View reviewed changes

yutakahirano mentioned this pull request Feb 8, 2021

Have "Handle Fetch" return error when request's streaming body is unusable w3c/ServiceWorker#1563

Merged

MattiasBuelens mentioned this pull request Feb 12, 2021

Use a ReadableStream with byte source (formerly called ReadableByteStream) for .body #267

Closed

Yutaka Hirano added 7 commits February 16, 2021 20:13

Fail request when streaming request body is used in ServiceWorker

9409f11

fix

7f17d36

fix

fd4a458

fix

ae0c430

fix

96ab414

fix

123d89d

fix

1705760

yutakahirano force-pushed the yhirano/streaming-upload-sw branch from 847abbc to 1705760 Compare February 16, 2021 11:17

annevk deleted the yhirano/streaming-upload-sw branch February 18, 2021 13:40

jakearchibald pushed a commit to w3c/ServiceWorker that referenced this pull request Feb 18, 2021

Have "Handle Fetch" return error when request's streaming body is unu…

7777030

…sable (#1563) Part of whatwg/fetch#1144

chromium-wpt-export-bot mentioned this pull request Feb 24, 2021

Make streaming upload work with network fallback on service worker web-platform-tests/wpt#27756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tee request body on ServiceWorker interception #1144

Tee request body on ServiceWorker interception #1144

yutakahirano commented Jan 22, 2021 •

edited by pr-preview bot

Loading

yutakahirano commented Jan 22, 2021

annevk commented Jan 22, 2021

yutakahirano commented Jan 22, 2021

annevk left a comment

yutakahirano commented Jan 27, 2021

annevk commented Jan 28, 2021

yutakahirano commented Jan 28, 2021

annevk commented Jan 28, 2021

yutakahirano commented Jan 28, 2021

jakearchibald left a comment

jakearchibald Jan 28, 2021

yutakahirano Jan 28, 2021

jakearchibald Jan 29, 2021

annevk Jan 29, 2021

yutakahirano Feb 3, 2021

yutakahirano Feb 12, 2021

annevk Feb 12, 2021

yoichio Feb 15, 2021

annevk Feb 15, 2021

yutakahirano Feb 16, 2021 •

edited

Loading

annevk commented Feb 16, 2021

yutakahirano commented Feb 16, 2021

annevk commented Feb 16, 2021

yutakahirano commented Feb 16, 2021

annevk commented Feb 16, 2021

yutakahirano commented Feb 18, 2021

Tee request body on ServiceWorker interception #1144

Tee request body on ServiceWorker interception #1144

Conversation

yutakahirano commented Jan 22, 2021 • edited by pr-preview bot Loading

yutakahirano commented Jan 22, 2021

annevk commented Jan 22, 2021

yutakahirano commented Jan 22, 2021

annevk left a comment

Choose a reason for hiding this comment

yutakahirano commented Jan 27, 2021

annevk commented Jan 28, 2021

yutakahirano commented Jan 28, 2021

annevk commented Jan 28, 2021

yutakahirano commented Jan 28, 2021

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutakahirano Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

annevk commented Feb 16, 2021

yutakahirano commented Feb 16, 2021

annevk commented Feb 16, 2021

yutakahirano commented Feb 16, 2021

annevk commented Feb 16, 2021

yutakahirano commented Feb 18, 2021

yutakahirano commented Jan 22, 2021 •

edited by pr-preview bot

Loading

yutakahirano Feb 16, 2021 •

edited

Loading