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

Define failure scenario timing resolution for same origin and T… #67

Merged
merged 1 commit into from
Sep 20, 2016

Conversation

wesleyhales
Copy link
Contributor

@wesleyhales wesleyhales commented Aug 19, 2016

wip #12
Links still need to be added, but I wanted to get feedback on wording first

@wesleyhales wesleyhales changed the title #12 wip - Define failure scenario timing resolution for same origin and T… Define failure scenario timing resolution for same origin and T… Aug 19, 2016
@@ -565,6 +565,7 @@
The time immediately after the user agent receives the last byte of the response or immediately before the transport connection is closed, whichever comes first. The resource here can be received either from <a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk what happens in Fetch if the server closes connection mid-stream (e.g. before all the Content-Length bytes are sent)? Will that trigger "network error" and reject the response promise?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not currently. And synthetic responses / content codings make this header unreliable.

If user agents were to implement such logic we might add it, but it seems that for something that streams for a long time, it would be a rather disastrous user experience.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. @plehegar do you, by any chance, remember why the "or immediately before the transport connection is closed" was added here / was meant to capture? Was this a clause for HTTP/1.0 and lack of keep-alive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was introduced following #25 , ie to sync with nav-timing. And this was in nav-timing since October 2010 ( https://dvcs.w3.org/hg/webperf/rev/3bb1ac67bfb2 ), based on the f2f ( https://lists.w3.org/Archives/Public/public-web-perf/2010Oct/0016.html ). It says "Add support for scenario where connection is closed."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting thanks Philippe! I'll go on the assumption that it was lack-of-keepalive related.

@igrigorik
Copy link
Member

👍 ... I think it's on the right track.

@annevk
Copy link
Member

annevk commented Aug 23, 2016

(I guess actually adding the logic into Fetch is the next step?)

@@ -802,7 +797,7 @@
starts the domain name lookup, record the time as <a for="PerformanceResourceTiming">domainLookupStart</a>.</li>
<li>Record the time as <a for="PerformanceResourceTiming">domainLookupEnd</a> immediately after the
domain name lookup is successfully done. A user agent may need multiple retries before that. If
the domain lookup fails, record the time immediately before the user agent aborts the same domain or Timing Allow Origin enabled request for resources, otherwise abort the remaining steps.</li>
the domain lookup fails, record the time as domainLookupEnd immediately after the domain name lookup is successfully done, or if the lookup fails and resource passes the <a>timing allow check</a>. A user agent may need multiple retries before that. If the domain lookup fails, set to zero and <a href="#step-final-queue">queue the record</a>.</li>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igrigorik How do we handle recording the duration for the final condition? Since Step 17 does not happen, should we set duration here or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Set to zero and go to step 18" instead of calling queue directly?

Sorry about the confusion, reviewing the preview once more:

  • Current steps 1-10 are setup.
  • 11-18 is where all the timestamps are recorded.
    • 12: Record the time as domainLookupEnd immediately after the domain name lookup is successfully done. A user agent may need multiple retries before that. If the domain name lookup fails and resource passes the timing allow check, record the time as domainLookupEnd and go to step 18.
    • 13-17... ~similar as above
    • 18: If responseEnd end is not set, set it to current time. Record the difference between responseEnd and startTime in duration.
    • ... rest as usual.

With above, I think we can also drop step 7 (in which case offset the step numbers above).

WDYT, does that look more reasonable?

Copy link
Contributor Author

@wesleyhales wesleyhales Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM 👍

@igrigorik
Copy link
Member

(I guess actually adding the logic into Fetch is the next step?)

This change is targeting L2, which doesn't integrate with Fetch. In L3 we'll refactor this to hook up with Fetch.

@@ -823,7 +823,7 @@
<li id="step-response-end">Record the time as <a for="PerformanceResourceTiming">responseEnd</a>
immediately after receiving the last byte of the response.
<ol>
<li>Return to step <a href="#step-connect-start">13</a> if the user agent fails to send the request or receive the entire response, and needs to reopen the connection.
<li>Return to step <a href="#step-connect-start">12</a> if the user agent fails to send the request or receive the entire response, and needs to reopen the connection.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the UA does not try to reopen the connection... e.g. closes it or times out on the response? Should we say something like "...if the user agent does not attempt to reopen the connection proceed to step 17?"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think current wording is fine. If it doesn't need/want to reopen it'll just continue and set responseEnd on 17?

@igrigorik
Copy link
Member

Overall, I think this gets us what we ~want.. A few observations:

  • we don't have secureConnectionEnd, which means we don't have a clear way to signal end of unsuccessful TLS handshake. I don't think this is something we can or should address in L2 though.
  • after re-reading the processing model, we can (read should) do some refactoring there.. But, if we're doing that, we might as well refactor on top of fetch, which we already slated for L3.

@toddreifsteck can you take a pass?

(p.s. I'm wondering if we should move this whole thing to L3 and pair it with fetch refactor)

@toddreifsteck
Copy link
Member

This update looks good to me. I'm ok with taking the change now or waiting till we refactor.

@wesleyhales
Copy link
Contributor Author

@igrigorik what's the next step from here? Should I squash my commits or will you handle on the merge?

@igrigorik
Copy link
Member

@wesleyhales apologies about the delay, been offline for the past few weeks. Yes, please squash.

Re, L2/L3: I propose we create an L3 branch and merge this into L3. We can then tackle the Fetch rewrite in a later commit. The reason for L3 is that this new behavior would not pass on any existing L2 implementations, and I think we want to stabilize and ship L2 CR sooner rather than later; we can shift new features into L3.

@wesleyhales
Copy link
Contributor Author

wesleyhales commented Sep 14, 2016

@igrigorik Squashed. Should I submit a new PR to an L3 branch? Shall I go ahead and create it?

@igrigorik
Copy link
Member

@plehegar my preference would be to keep latest draft on master branch? Should we create an L2 branch and iterate there, and merge this into master / treat it as L3? WDYT?

@igrigorik igrigorik merged commit f3c186b into w3c:gh-pages Sep 20, 2016
triple-underscore added a commit to triple-underscore/triple-underscore.github.io that referenced this pull request Sep 22, 2016
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

Successfully merging this pull request may close these issues.

5 participants