Enforce limit on inflight keepalive bytes #419

igrigorik · 2016-11-23T20:16:32Z

Requests with keepalive flag set are allowed to outlive the environment
settings object. We want to make sure that such requests do not
negatively impact the user experience when a page is unloaded, etc.

This limits the amount of (body) bytes that can be inflight at any point
when the request has the keepalive flag set; this flag is set by
sendBeacon.

Background: w3c/beacon#39

Preview | Diff

annevk · 2016-11-24T08:05:30Z

fetch.bs

@@ -3122,10 +3122,17 @@ steps:

 <li>
  <p>If <var>contentLengthValue</var> is non-null, <var>httpRequest</var>'s
-  <a>keepalive flag</a> is set, and <var>contentLengthValue</var> is greater than a
-  user-agent-defined maximum, then return a <a>network error</a>.
+  <a>keepalive flag</a> is set:


annevk · 2016-11-24T08:10:24Z

fetch.bs

+  <ul>
+    <li>Let <var>group</var> be <var>httpRequest</var>'s <a>client</a>'s <a>fetch group</a>.
+    <li>Let <var>inflight keepalive bytes</var> be the sum of <a>request</a> <a>body</a>'s
+    <a>total bytes</a> for each <a>record</a> in <var>group</var> whose <a>done flag</a> is unset.


I get what you're trying to say, but this sentence does not read well. Maybe "sum of each X's Y in Z"?

I also think this is lacking some for="" attributes since the request you're talking about here is that of the fetch group, as is the record.

annevk · 2016-11-24T08:11:00Z

fetch.bs

+    <li>Let <var>inflight keepalive bytes</var> be the sum of <a>request</a> <a>body</a>'s
+    <a>total bytes</a> for each <a>record</a> in <var>group</var> whose <a>done flag</a> is unset.
+    <li>If the sum of <var>contentLengthValue</var> and <var>inflight keepalive bytes</var> is
+    greater than 64KB, then return a <a>network error</a>.


KiB, presumably? I wonder if we should use <abbr> for the occasion.

igrigorik · 2016-11-28T18:00:12Z

@annevk ack, updated. I've split up the sum step.. hopefully that makes it more clear.

annevk · 2016-11-29T08:25:41Z

You need to use <ol> instead of <ul> and generally things are indented with only a single space and <li> are separated by a newline (though not from the opening <ol> and closing </ol>).

I also think some steps can be grouped together again, but I'm happy to make that change later on.

Is there someone else that can review this? And do we have tests for this somewhere?

igrigorik · 2016-11-29T15:54:29Z

@tyoshino could you please take a look?

@annevk please feel free to wordsmith as needed.

tyoshino · 2016-11-30T09:37:14Z

looks good

annevk · 2016-11-30T10:29:04Z

To be clear, still need an update on the testing scenario.

igrigorik · 2016-11-30T16:23:08Z

@annevk sorry, forgot about that. Are you looking for web-platform tests? No one has implemented this yet, so no, we don't have those.. yet. We (Chrome) are looking to implement this (and associated sendBeacon changes) in short order, so that should be resolved fairly soon.

igrigorik · 2016-12-05T16:54:44Z

bump

@annevk anything else I can help unblock on this one?

annevk · 2017-01-12T13:02:45Z

I'm sorry. I totally forgot about this. Yeah I am looking for web-platform-tests. If they are being written as part of the implementation I suppose that might be sufficient, though not ideal. Perhaps a simple test can be added?

We have a new policy where each standard PR also has a WPT PR and then the former lands after the latter.

igrigorik · 2017-02-11T08:37:40Z

@annevk PTAL: web-platform-tests/wpt#4788

annevk · 2017-02-13T13:16:24Z

Thanks, I'll rebase this PR meanwhile. Tests can probably land pretty quickly if nobody else has feedback and my minor nits are fixed.

annevk · 2017-02-13T13:20:32Z

Hmm, I'm not sure how to rebase this since you added the commits on master.

annevk · 2017-02-16T08:38:44Z

The tests moved to web-platform-tests/wpt#4878. @igrigorik I'm still blocked on you sorting out how to rebase this.

Requests with keepalive flag set are allowed to outlive the environment settings object. We want to make sure that such requests do not negatively impact the user experience when a page is unloaded, etc. This limits the amount of (body) bytes that can be inflight at any point when the request has the keepalive flag set; this flag is set by sendBeacon. Background: w3c/beacon#39

- split sum step into a for loop - added group definition - use KiB

igrigorik · 2017-02-24T17:06:52Z

@annevk apologies about the delay. Updated.. ptal.

annevk · 2017-02-24T17:54:20Z

I made some formatting changes. Please review. Also note that we're currently blocked on speced/bikeshed#936.

igrigorik · 2017-02-24T18:01:13Z

@annevk lgtm, good to merge once Bikeshed issue is resolved.

annevk · 2017-02-24T18:03:39Z

@igrigorik also, it's fine filing bugs now we have an agreed upon standard change and tests. Can you do that? Just file a bug against everyone that hasn't implemented this with a pointer to this PR and the tests.

igrigorik · 2017-02-24T18:15:59Z

AFAIK, Edge already has it implemented.

Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=695939
Mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1342484
Webkit: https://bugs.webkit.org/show_bug.cgi?id=168865

annevk · 2017-02-25T06:10:50Z

We also need one for WebKit, right?

igrigorik · 2017-02-25T06:23:54Z

@annevk indeed. Opened & updated the list above.

annevk · 2017-02-25T06:25:08Z

I'd also appreciate one more final review since I had to make a couple more changes for Bikeshed.

igrigorik · 2017-02-25T06:30:37Z

Took another scrub.. LGTM.

Initial set of quota tests for keepalive fetch requests. See whatwg/fetch#419 for details.

annevk · 2017-02-25T07:40:20Z

Great, thanks for reporting the bugs!

mnot · 2017-02-27T06:32:52Z

I don't suppose a different name for this could be used? Calling it "keepalive" is going to be confusing for some HTTP people...

annevk · 2017-02-27T12:27:34Z

I don't think any code has shipped thus far, though Edge has implemented. So if we have a better name we could rename, but we'd have to be quick I think and @igrigorik should confirm.

igrigorik · 2017-02-27T16:44:35Z

@mnot it's request level keepalive... which—I think—is a good semantic fit with what we're trying to communicate here. I don't think we have have to monopolize keepalive for connection level semantics; most developers will never touch connection keepalive.

mnot · 2017-02-27T23:21:58Z

most developers will never touch connection keepalive

That's a surprising assertion, but OK...

mnot · 2017-02-28T05:05:34Z

One more possibly stupid question - why is this in HTTP-network-or-cache fetch and not HTTP-network fetch?

annevk · 2017-02-28T11:42:00Z

No stupid questions. I don't think there's an observable difference and I suspect @igrigorik did it this way because the former already had a handle on the body size.

annevk requested changes Nov 24, 2016

View reviewed changes

annevk added the needs tests Moving the issue forward requires someone to write tests label Jan 17, 2017

igrigorik mentioned this pull request Feb 11, 2017

Enforce payload limits via Fetch API w3c/beacon#39

Merged

annevk mentioned this pull request Feb 16, 2017

Fetch KeepAlive Quota Tests web-platform-tests/wpt#4878

Merged

annevk removed the needs tests Moving the issue forward requires someone to write tests label Feb 16, 2017

igrigorik added 2 commits February 24, 2017 09:00

cleanup and clarify

3ec75a0

- split sum step into a for loop - added group definition - use KiB

igrigorik force-pushed the master branch from dd55e9b to 3ec75a0 Compare February 24, 2017 17:03

formatting nits

0227ce6

more nits (and some changes for changes made in IDL)

1bd7e76

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 25, 2017

Fetch keepalive quota tests

4d1e656

Initial set of quota tests for keepalive fetch requests. See whatwg/fetch#419 for details.

annevk merged commit 3b4cb54 into whatwg:master Feb 25, 2017

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce limit on inflight keepalive bytes #419

Enforce limit on inflight keepalive bytes #419

igrigorik commented Nov 23, 2016 •

edited by pr-preview bot

Loading

annevk Nov 24, 2016

annevk Nov 24, 2016

annevk Nov 24, 2016

igrigorik commented Nov 28, 2016

annevk commented Nov 29, 2016

igrigorik commented Nov 29, 2016

tyoshino commented Nov 30, 2016

annevk commented Nov 30, 2016

igrigorik commented Nov 30, 2016

igrigorik commented Dec 5, 2016

annevk commented Jan 12, 2017

igrigorik commented Feb 11, 2017

annevk commented Feb 13, 2017

annevk commented Feb 13, 2017

annevk commented Feb 16, 2017

igrigorik commented Feb 24, 2017

annevk commented Feb 24, 2017

igrigorik commented Feb 24, 2017

annevk commented Feb 24, 2017

igrigorik commented Feb 24, 2017 •

edited

Loading

annevk commented Feb 25, 2017

igrigorik commented Feb 25, 2017

annevk commented Feb 25, 2017

igrigorik commented Feb 25, 2017

annevk commented Feb 25, 2017

mnot commented Feb 27, 2017

annevk commented Feb 27, 2017

igrigorik commented Feb 27, 2017

mnot commented Feb 27, 2017

mnot commented Feb 28, 2017

annevk commented Feb 28, 2017

Enforce limit on inflight keepalive bytes #419

Enforce limit on inflight keepalive bytes #419

Conversation

igrigorik commented Nov 23, 2016 • edited by pr-preview bot Loading

annevk Nov 24, 2016

Choose a reason for hiding this comment

annevk Nov 24, 2016

Choose a reason for hiding this comment

annevk Nov 24, 2016

Choose a reason for hiding this comment

igrigorik commented Nov 28, 2016

annevk commented Nov 29, 2016

igrigorik commented Nov 29, 2016

tyoshino commented Nov 30, 2016

annevk commented Nov 30, 2016

igrigorik commented Nov 30, 2016

igrigorik commented Dec 5, 2016

annevk commented Jan 12, 2017

igrigorik commented Feb 11, 2017

annevk commented Feb 13, 2017

annevk commented Feb 13, 2017

annevk commented Feb 16, 2017

igrigorik commented Feb 24, 2017

annevk commented Feb 24, 2017

igrigorik commented Feb 24, 2017

annevk commented Feb 24, 2017

igrigorik commented Feb 24, 2017 • edited Loading

annevk commented Feb 25, 2017

igrigorik commented Feb 25, 2017

annevk commented Feb 25, 2017

igrigorik commented Feb 25, 2017

annevk commented Feb 25, 2017

mnot commented Feb 27, 2017

annevk commented Feb 27, 2017

igrigorik commented Feb 27, 2017

mnot commented Feb 27, 2017

mnot commented Feb 28, 2017

annevk commented Feb 28, 2017

igrigorik commented Nov 23, 2016 •

edited by pr-preview bot

Loading

igrigorik commented Feb 24, 2017 •

edited

Loading