http2: simplify timeout tracking by addaleax · Pull Request #19206 · nodejs/node · GitHub

Splitting out from #18936:

There’s no need to reset the chunk counter for every write.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

There’s no need to reset the chunk counter for every write.

CI: https://ci.nodejs.org/job/node-test-commit/16763/

@addaleax would you be so kind and check the CI? I have the feeling there might be related errors, e.g., https://ci.nodejs.org/job/node-test-commit-linux-containered/2768/nodes=ubuntu1604_sharedlibs_openssl110_x64/console.

@BridgeAR Thanks for checking! I don’t think the errors are related, because this only affects http2, whereas the test only uses the built-in http module.

But still, let’s start a new CI just to be sure: https://ci.nodejs.org/job/node-test-commit/16842/

Landed in 90e70b8

There’s no need to reset the chunk counter for every write. PR-URL: #19206 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

There’s no need to reset the chunk counter for every write. PR-URL: nodejs#19206 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This doesn't land cleanly on v8.x, please could you raise a backport PR?
Does this need to land with/alongside #22850? @addaleax @kjin

I can open a PR first thing tomorrow -- how did you find out that this commit wasn't backported? I'm wondering if there are others that slipped through the crack as well, somehow.

There’s no need to reset the chunk counter for every write. PR-URL: nodejs#19206 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@kjin Maybe the issue is that this wasn’t labeled http2…? I’m not sure what to do about that, though :/

@kjin As far as PRs that were labeled http2 go …

$ branch-diff upstream/v8.x-staging upstream/v10.x-staging --require-label=http2
[…]

[42135afd3c] - test: remove unused "e" from catch in http2 test ( Stephen Heitman) #23476
[7b327ea909] - (SEMVER-MINOR) http2: add RFC 8441 extended connect protocol support ( James M Snell) #23284
[95bdf37265] - (SEMVER-MINOR) deps: update nghttp2 to 1.34.0 ( James M Snell) #23284
[001881f33e] - http2: set nghttp2_option_set_no_closed_streams ( David Halls) #23134
[8fe62f8d38] - http2: don't send trailers on a closed connection ( André Cruz) #23146
[d1826fed41] - http2: close fd in doSendFileFD() ( cjihrig) #23047
[7c831b754a] - Working on v10.10.1 ( Michaël Zasso) #22716
[cdb359840c] - 2018-09-06, Version 10.10.0 (Current) ( Michaël Zasso) #22716
[2a88731ed7] - lib: merge onread handlers for http2 streams & net.Socket ( Ashok) #22449
[8d226c6a79] - http2: correcting the heading format ( Anto Aravinth) #22262
[c3d9000111] - doc: document http2 network error behaviour ( Anna Henningsen) #21861
[eea199bf98] - test: fix http2 connection abort test ( Anna Henningsen) #21861
[0b3c80ca31] - http2: fix issues with aborted respondWithFile()s ( Anna Henningsen) #21561
[e92b89a75d] - src: fix http2 typos ( Anatoli Papirovski) #21194
[182c73bf7f] - http2: switch to new runtime-controlled debugging system ( Anna Henningsen) #20987
[45eeea4330] - src: implement debug output utilities ( Anna Henningsen) #20987
[e397d19e58] - http2: remove unnecessary v8 qualified names ( Daniel Bevenius) #20420
[f086354d3b] - errors: alter ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED ( davidmarkclements) #19958
[e85c20b511] - net,http2: merge write error handling & property names ( Anna Henningsen) #19734
[cef909797a] - http2: do not emit our own close emit in Http2Stream ( James M Snell) #19451
[1ac1424476] - http: align parser with StreamBase interface changes ( Anna Henningsen) #18936
[e136903700] - benchmark: remove excessive value from http2 benchmark ( Anna Henningsen) #18936
[d93c48bf61] - src: use ObjectTemplate for creating stream req objs ( Anna Henningsen) #18936
[12b9ec09b0] - http2: remove regular-file-only restriction ( Anna Henningsen) #18936
[1eb6b01fca] - http2: use native pipe instead of synchronous I/O ( Anna Henningsen) #18936
[67f1d76956] - src: introduce native-layer stream piping ( Anna Henningsen) #18936
[f7f1437d44] - src: add helper for before/after scope without JS calls ( Anna Henningsen) #18936
[f734b3eb04] - src: give StreamBases the capability to ask for data ( Anna Henningsen) #18936
[c412150582] - src: make FileHandle a (readonly) StreamBase ( Anna Henningsen) #18936
[8695273948] - src: tighten handle scopes for stream operations ( Anna Henningsen) #18936
[8403f00dc3] - http2: fixes error handling ( Matteo Collina) #19232
[90e70b8caf] - http2: simplify timeout tracking ( Anna Henningsen) #19206
[5e3f51648e] - (SEMVER-MAJOR) stream: updated streams error handling ( Mathias Buus) #18438
[197fbbed55] - 2018-03-01, Version 9.7.0 (Current) ( Rod Vagg) #19040
[bb5575aa75] - timers: add internal [@@ refresh()] function ( Jeremiah Senkpiel) #18065
[54fe0a6cbb] - timers: reposition getTimers definition internally ( Jeremiah Senkpiel) #18065
[0812ebda88] - http2: fix kUpdateTimer timer refresh ( Jeremiah Senkpiel) #18062
[e688175c50] - 2019-01-10 Version 9.4.0 (Current) ( Myles Borins) #18069
[fc61ee32fe] - (SEMVER-MAJOR) http2: use session kUpdateTimer from kUpdateTimer ( Jeremiah Senkpiel) #17704
[93eb68e6d2] - (SEMVER-MAJOR) http2: use actual Timeout instances ( Jeremiah Senkpiel) #17704
[593941ac0b] - (SEMVER-MAJOR) timers: extract enroll() validation into a fn ( Jeremiah Senkpiel) #17704
[24dd92e77f] - (SEMVER-MAJOR) net: use actual Timeout instance on Sockets ( Jeremiah Senkpiel) #17704
[f912080bf2] - Revert "http2: refactor error handling" ( Rich Trott) #15047

There’s no need to reset the chunk counter for every write. Backport-PR-URL: #23774 PR-URL: #19206 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

http2: simplify timeout tracking

4eb9b67

There’s no need to reset the chunk counter for every write.

addaleax requested a review from jasnell March 7, 2018 17:36

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 7, 2018

addaleax requested a review from apapirovski March 7, 2018 17:37

jasnell approved these changes Mar 7, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 7, 2018

danbev approved these changes Mar 8, 2018

View reviewed changes

apapirovski approved these changes Mar 8, 2018

View reviewed changes

cjihrig approved these changes Mar 8, 2018

View reviewed changes

addaleax closed this Mar 11, 2018

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 11, 2018

addaleax deleted the http2-simplify-timeout-tracking branch March 11, 2018 19:16

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

BethGriggs added the backport-requested-v8.x label Oct 17, 2018

kjin mentioned this pull request Oct 20, 2018

[v8.x backport] http2: simplify timeout tracking #23774

Closed

2 tasks

addaleax added the http2 Issues or PRs related to the http2 subsystem. label Oct 20, 2018

BethGriggs mentioned this pull request Nov 12, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

http2: simplify timeout tracking by addaleax · Pull Request #19206 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: simplify timeout tracking - #19206

http2: simplify timeout tracking#19206
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:http2-simplify-timeout-tracking

addaleax commented Mar 7, 2018

Uh oh!

addaleax commented Mar 7, 2018

Uh oh!

BridgeAR commented Mar 11, 2018

Uh oh!

addaleax commented Mar 11, 2018

Uh oh!

addaleax commented Mar 11, 2018 •
edited
Loading

Uh oh!

Uh oh!

BethGriggs commented Oct 17, 2018

Uh oh!

kjin commented Oct 18, 2018

Uh oh!

addaleax commented Oct 20, 2018

Uh oh!

addaleax commented Oct 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

http2: simplify timeout tracking by addaleax · Pull Request #19206 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: simplify timeout tracking - #19206

http2: simplify timeout tracking#19206addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:http2-simplify-timeout-trackingaddaleax/node:http2-simplify-timeout-trackingCopy head branch name to clipboard

Conversation

addaleax commented Mar 7, 2018

Checklist

Uh oh!

addaleax commented Mar 7, 2018

Uh oh!

BridgeAR commented Mar 11, 2018

Uh oh!

addaleax commented Mar 11, 2018

Uh oh!

addaleax commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BethGriggs commented Oct 17, 2018

Uh oh!

kjin commented Oct 18, 2018

Uh oh!

addaleax commented Oct 20, 2018

Uh oh!

addaleax commented Oct 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

http2: simplify timeout tracking#19206
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:http2-simplify-timeout-tracking

addaleax commented Mar 11, 2018 •
edited
Loading