allow Request to outlive environment settings object #388

igrigorik · 2016-09-16T16:58:37Z

Related discussion in w3c/beacon#27.

@annevk ptal, is this on the right track? I have a few ???'s and TODO's that I could use help with :)

annevk · 2016-09-20T07:49:13Z

I'm not entirely sure actually. @yutakahirano can hopefully help.

yutakahirano · 2016-09-20T12:46:21Z

concept-request relies on ReadableStream which relies on the realm. So we cannot read data from the request body when the environment settings object is invalid, I guess. cc: @domenic

domenic · 2016-09-20T13:21:01Z

Yeah, that's true, if someone sends a ReadableStream body but the window has been collected, you're in trouble.

igrigorik · 2016-09-20T17:49:03Z

Ah, well.. In that case I guess we should throw an error if body object's type is ReadableStream and keepAlive flag is set? In effect, if you want keepAlive semantics than you have to provide the body upfront. Also, that also eliminates the timer question.

Before I update the pull.. Does that make sense? Any other gotchas?

igrigorik · 2016-09-22T08:49:53Z

TPAC notes:

Streams are still work-in-progress, for v1 we can throw an error if body is a ReadableStream and keepAlive flag is enabled (same behavior for upload streams)
Throw TypeError if/when size is exceeded.
Redirects are handled by Fetch, no changes needed.
Fetch will return a promise and opaque body when mode is no-cors: this is fine.

Adding keepAlive semantics gets us most of the way there. As a separate topic we should also followup on #184 (coalescing).

yutakahirano · 2016-09-23T02:07:30Z

The problem is that the concept layer also depends on ReadableStream. Even when you create a Request object from a DOMString body, the body is held as a ReadableStream (see https://fetch.spec.whatwg.org/#concept-body and https://fetch.spec.whatwg.org/#concept-bodyinit-extract).

If we need to have a fetch operation work beyond the realm destruction, we need to define a stream that works in such a circumstance.

wanderview · 2016-09-23T02:13:30Z

The problem is that the concept layer also depends on ReadableStream. Even when you create a Request object from a DOMString body, the body is held as a ReadableStream (see https://fetch.spec.whatwg.org/#concept-body and https://fetch.spec.whatwg.org/#concept-bodyinit-extract).

I thought one of the goals was to let the browser optimize such that if it never has to execute js to read the stream, then it could perform off thread copying, etc. A DOMString body should fall into this category, no?

igrigorik · 2016-09-29T16:39:03Z

Updated / simplified the proposed change in 3b71edd -- getting warmer?

This doesn't address @yutakahirano's comment about Streams support for outliving real destruction.. but that smells like a Streams API feature? @domenic any thoughts or recommendations on this one?

domenic · 2016-09-29T17:08:03Z

Yeah, I'm not sure what to do about that issue :-/. I think it might be worth noting in the spec somehow, and I'll try to think of a nicer solution next week...

igrigorik · 2016-09-29T17:42:02Z

@domenic sgtm, thanks!
@annevk how do you feel about current changeset? Flag name, processing logic, etc.

annevk

Many nits, but the main problem I see here is that you haven't defined what size of an object means. As I understand it from a discussion with @esprehn and others, calculating the size of FormData requires a roundtrip through networking land. Whereas this would require that to be a synchronous lookup.

annevk · 2016-09-30T09:00:13Z

Overview.src.html

@@ -4224,7 +4224,7 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul

 <p>To <dfn title=concept-BodyInit-extract>extract</dfn> a <span title=concept-body>body</span> and a
 `<code title>Content-Type</code>` <span title=concept-header-value>value</span> from
-<var>object</var>, run these steps:
+<var>object</var> with optional <var>keepalive-body-size</var>, run these steps:


keepaliveBodySize (Content-Type is an exception that should maybe be modified at some point).

annevk · 2016-09-30T09:01:26Z

Overview.src.html

@@ -4487,6 +4493,7 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
  readonly attribute <span>RequestCache</span> <span title=dom-Request-cache>cache</span>;
  readonly attribute <span>RequestRedirect</span> <span title=dom-Request-redirect>redirect</span>;
  readonly attribute DOMString <span title=dom-Request-integrity>integrity</span>;
+  readonly attribute boolean <span title=dom-Request-keepAlive>keepAlive</span>;


keepAlive or keepalive? We should stay consistent. https://en.wikipedia.org/wiki/Keepalive suggests the time has come where it is one word, but HTTP sets precedent in the other direction unfortunately.

I'll rename to lowercase "keepalive" throughout.

annevk · 2016-09-30T09:02:37Z

Overview.src.html

 <span title=concept-request-integrity-metadata>integrity metadata</span> is
 <var>request</var>'s
- <span title=concept-request-integrity-metadata>integrity metadata</span>.
+ <span title=concept-request-integrity-metadata>integrity metadata</span>, and
+ <span title=concept-request-keep-alive-flag>keep-alive flag</span> is


This flag hasn't been defined it seems?

A request has an associated keep-alive flag. Unless stated otherwise it is unset.

I think that should do the trick?

Yeah, though "keepalive" per earlier comment and maybe a note following it that describes it would be good. Ideally all request fields have a note describing them, but I haven't gotten around to do that just yet.

annevk · 2016-09-30T09:02:58Z

Overview.src.html

@@ -4734,6 +4746,10 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
 <var>request</var>'s
 <span title=concept-request-integrity-metadata>integrity metadata</span> to it.

+ <li><p>If <var>init</var>'s <code title>keepAlive</code> member is present, set


I can change that, but (fyi) it's inconsistent with surrounding language.. e.g.

If init's integrity member is present, set request's integrity metadata to it.

I'll keep as is for now, let me know if you want me to change it.

Please change it. Existing language should also be updated at some point. But it's better if new language does it from the start for git blame and such.

annevk · 2016-09-30T09:03:38Z

Overview.src.html

@@ -4291,6 +4291,12 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
  </dl>

 <li>
+  <p>If <var>keepalive-body-size</var> is provided and <var>object</var>'s type is
+  <code title=concept-ReadableStream>ReadableStream</code>, or <var>object</var>'s size is greater
+  than <var>keepalive-body-size</var>, throw a <code title>TypeError</code> and abort the


then throw* and no need to say the remaining steps are aborted. That follows from throwing (new convention we're trying to use).

annevk · 2016-09-30T09:04:41Z

Overview.src.html

+   <li><p>If <var>init</var>'s <code title>keepAlive</code> member is present and is set to
+   <code>true</code>, then set <var>inputBody</var> and <var>Content-Type</var> to the
+   result of <span title=concept-BodyInit-extract>extracting</span> <var>init</var>'s
+   <code title>body</code> member with <var>keepalive-body-size</var> set by user agent policy.


I suggest we introduce keepalive-body-size in an earlier step so it's more clear it's user-agent defined.

annevk · 2016-09-30T09:05:17Z

Overview.src.html

@@ -4802,7 +4818,17 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
  <ol>
   <li><p>Let <var>Content-Type</var> be null.

-   <li><p>Set <var>inputBody</var> and <var>Content-Type</var> to the result of
+   <li><p>If <var>init</var>'s <code title>keepAlive</code> member is present and is set to


You need a newline after the <li> since it contains two <p>s.

annevk · 2016-09-30T09:05:36Z

Overview.src.html

+   result of <span title=concept-BodyInit-extract>extracting</span> <var>init</var>'s
+   <code title>body</code> member with <var>keepalive-body-size</var> set by user agent policy.
+   Rethrow any exceptions.
+   <p class="note no-backref">This step ensures that requests that are allowed to outlive the


Newline before the <p> and probably also should be indented one space more.

annevk · 2016-09-30T09:06:32Z

Overview.src.html

@@ -4291,6 +4291,12 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
  </dl>

 <li>
+  <p>If <var>keepalive-body-size</var> is provided and <var>object</var>'s type is
+  <code title=concept-ReadableStream>ReadableStream</code>, or <var>object</var>'s size is greater


a ReadableStream object*

Also, how do we determine object's size? As I understand it that can be quite hard for FormData.

igrigorik · 2016-09-30T17:38:36Z

@annevk thanks for the review, I think I addressed most of it in 759a8fa, modulo...

Also, how do we determine object's size? As I understand it that can be quite hard for FormData.

That's a great question.. I was hoping you could point me towards a handy hook somewhere. :)

How does 5.5 step 5 derive it?

If httpRequest's body is non-null, set contentLengthValue to httpRequest's body's total bytes, utf-8 encoded.

Presumably this step has the same issue as you outlined above?

annevk · 2016-10-03T09:20:22Z

Yeah, I think it does. The main difference is that currently we don't need to know the size synchronously as it's not exposed anywhere. This would expose it synchronously and I don't think user agents would be happy with that.

igrigorik · 2016-10-03T16:08:22Z

That's true. Well, I guess the other approach we have here is:

Keep the existing logic to throw TypeError if body is a ReadableStream.
Add new step in HTTP-network-or-cache fetch after we obtain contentLengthValue, which checks that the size is less than the ~UA-set size for requests with keepalive flag set. If the condition doesn't hold, return a network error.

~section 5.5, step 7:

If contentLengthValue is non-null, the httpRequest's keepalive flag is set, and contentLengthValue is greater than the UA-set maximum body size for such requests, return a network error.

WDYT?

annevk · 2016-10-04T07:38:40Z

"user-agent-defined maximum body size", but yes, that could work. I'd love @esprehn or someone else from Blink to sign off on that though.

annevk · 2016-10-04T07:39:55Z

Overview.src.html

 <span title=concept-request-integrity-metadata>integrity metadata</span> is
 <var>request</var>'s
- <span title=concept-request-integrity-metadata>integrity metadata</span>.
+ <span title=concept-request-integrity-metadata>integrity metadata</span>, and
+ <span title=concept-request-keepalive-flag>keepalive flag</span> is


Note that keepalive flag doesn't need a title since the <dfn> doesn't have one either. I should probably have given all flags a title from the start, but too late now and when it's all converted to bikeshed at some point it'll no longer matter much.

annevk · 2016-10-04T07:40:10Z

Overview.src.html

+ <span title=concept-request-integrity-metadata>integrity metadata</span>, and
+ <span title=concept-request-keepalive-flag>keepalive flag</span> is
+ <var>request</var>'s
+ <span title=concept-request-keepalive-flag>keepalive flag</span>


title can be removed here too.

annevk · 2016-10-04T07:40:16Z

Overview.src.html

@@ -4734,6 +4746,10 @@ <h4 id="should-response-to-request-be-blocked-due-to-nosniff?"><dfn title="shoul
 <var>request</var>'s
 <span title=concept-request-integrity-metadata>integrity metadata</span> to it.

+ <li><p>If <var>init</var>'s <code title>keepalive</code> member is present, then set
+ <var>request</var>'s
+ <span title=concept-request-keepAlive>keepalive flag</span> to it.


igrigorik · 2016-10-04T16:29:19Z

@annevk updated, PTAL. In the meantime, I'll ping some Blink folks for a review -- stay tuned.

esprehn · 2016-10-05T01:39:35Z

Yeah I think that works. Btw what makes it clear that the fetch result promise will never resolve on a keepalive request if the page is destroyed?

annevk · 2016-10-05T09:19:31Z

I don't think anything at the moment. We don't have a clear "page is destroyed" hook yet. What does that mean, by the way? Browsing context going away (iframe element removal), navigation, something else? There's a lot around the lifetime of the various global objects that's extremely poorly defined at the moment.

tyoshino · 2016-10-12T05:11:20Z

the spec change lgtm. we still have issues with our implementation for the sendBeacon and this, but I agree this is what we should pursue. I guess Chromium could start implementing this in Q117.

annevk · 2016-10-12T07:50:03Z

@youennf any feedback from WebKit?

Apart from some minor nits I can take care of when merging this looks okay to me now. (The lifetime issues will need to be sorted, but they are larger than this issue in my estimation.)

youennf · 2016-10-12T11:04:48Z

Makes sense to me.
I am not sure how we might handle the FormData case in WebKit though.

I guess the keep-alive attribute might be useful for service workers although for some of the keep-alive use-cases, there might be no need to resolve/create any response.

igrigorik · 2016-10-12T15:19:38Z

@tyoshino @youennf thanks for the review! @annevk sounds like you got it from here.. that said, let me know if you want my help with anything.

Related discussion in [1]. This exposes keepAlive flag within Request constructor and adds guards for limiting the size of such requests. [1] w3c/beacon#27

annevk · 2016-10-13T08:38:07Z

@igrigorik the only thing that remains doing is writing tests. I assume that will happen as part of implementation work, but if you all could make sure they end up in https://github.com/w3c/web-platform-tests that'd be great.

annevk · 2016-10-13T08:38:49Z

Also, thank you for your work on this!

igrigorik · 2016-10-13T15:32:02Z

@annevk yep, thanks! I'll a tests TODO on my list..

wanderview · 2016-10-13T16:13:58Z

I'm sorry I didn't provide more feedback sooner, but can someone explain why we need to expose a keepalive flag to script? Why can't the browser just consume the Request immediately when sendBeacon() is called, store an internal request, and do the fetch later?

It feels really weird to me to ask devs to specify a flag for something the browser can probably do automatically in implementation.

annevk · 2016-10-13T16:19:18Z

How would this work with sendBeacon()? E.g., what this enables is a keepalive HEAD. Not possible today.

wanderview · 2016-10-13T17:14:23Z

How would this work with sendBeacon()?

The same way we effectively keep Request objects in Cache API beyond the end of an environment settings object. The browser has to maintain an internal representation in some way. It doesn't need to keep the JS object alive.

But maybe I don't understand what you mean by "keepalive HEAD".

annevk · 2016-10-13T17:17:35Z

E.g., how would you write fetch(url, {keepalive:true, method:"HEAD"}) using the navigator.sendBeacon() API?

wanderview · 2016-10-13T17:32:00Z

Anne explained further in IRC that this is about not canceling the fetch operation if the environment settings object is destroyed. I got confused because the PR and spec text talk more about the lifecycle of (R|r)equest objects.

Anyway, carry on.

igrigorik · 2016-10-18T17:13:44Z

From w3c/beacon#27 (comment):

I realize I'm late to the party here, but IMO it would have been nicer to make sendBeacon() drain the Request to an internal storage space and then send it out later from there. Then the Request and its JS context don't need to stick around unless the upload body is a JS sourced stream.

@tyoshino @annevk @youennf any thoughts or opinions on the above?

annevk · 2016-10-18T17:15:34Z

I think @wanderview misunderstood what was going on due to some unclear wording in this PR's title. See comments above.

igrigorik · 2016-10-18T17:18:11Z

OK, never mind.. Onwards! :-)

annevk reviewed Sep 30, 2016

View reviewed changes

annevk reviewed Oct 4, 2016

View reviewed changes

igrigorik changed the title ~~(WIP) allow Request to outlive environment settings object~~ allow Request to outlive environment settings object Oct 4, 2016

igrigorik added 4 commits October 13, 2016 09:49

allow Request to outlive environment settings object

84154b9

Related discussion in [1]. This exposes keepAlive flag within Request constructor and adds guards for limiting the size of such requests. [1] w3c/beacon#27

disallow ReadableStream if keepAlive flag is set

164aaa0

integrate Anne's feedback

9a9847b

s/keep-alive/keepalive + extended flag description

1ce51bf

igrigorik and others added 2 commits October 13, 2016 09:49

add body size check in HTTP-network-or-cache fetch

97c6eca

nits

75f1982

annevk force-pushed the keepAlive branch from 08fbc49 to 75f1982 Compare October 13, 2016 07:50

preserve old keep-alive ID plus more nits plus generate

6307581

annevk merged commit 5d745b2 into whatwg:master Oct 13, 2016

This was referenced Oct 13, 2016

Add method to sendBeacon w3c/beacon#27

Closed

Allow setting HTTP(S) headers in beacon w3c/beacon#30

Closed

Should sendBeacon support GET? w3c/beacon#22

Closed

domenic mentioned this pull request Nov 4, 2016

Streams and fetches that outlive the environment settings object #411

Closed

allow Request to outlive environment settings object #388

allow Request to outlive environment settings object #388

Conversation

igrigorik commented Sep 16, 2016

annevk commented Sep 20, 2016

yutakahirano commented Sep 20, 2016

domenic commented Sep 20, 2016

igrigorik commented Sep 20, 2016

igrigorik commented Sep 22, 2016 • edited Loading

yutakahirano commented Sep 23, 2016 • edited Loading

wanderview commented Sep 23, 2016

igrigorik commented Sep 29, 2016

domenic commented Sep 29, 2016

igrigorik commented Sep 29, 2016

annevk 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

annevk Sep 30, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrigorik commented Sep 30, 2016

annevk commented Oct 3, 2016

igrigorik commented Oct 3, 2016

annevk commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrigorik commented Oct 4, 2016

esprehn commented Oct 5, 2016

annevk commented Oct 5, 2016

tyoshino commented Oct 12, 2016

annevk commented Oct 12, 2016 • edited Loading

youennf commented Oct 12, 2016

igrigorik commented Oct 12, 2016

annevk commented Oct 13, 2016

annevk commented Oct 13, 2016

igrigorik commented Oct 13, 2016

wanderview commented Oct 13, 2016

annevk commented Oct 13, 2016

wanderview commented Oct 13, 2016

annevk commented Oct 13, 2016

wanderview commented Oct 13, 2016

igrigorik commented Oct 18, 2016

annevk commented Oct 18, 2016

igrigorik commented Oct 18, 2016

igrigorik commented Sep 22, 2016 •

edited

Loading

yutakahirano commented Sep 23, 2016 •

edited

Loading

annevk Sep 30, 2016 •

edited

Loading

annevk commented Oct 12, 2016 •

edited

Loading