Skip to content

http2: simplify timeout tracking #19206

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

http2: simplify timeout tracking #19206

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Mar 7, 2018

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

@addaleax
http2: simplify timeout tracking
4eb9b67 
There’s no need to reset the chunk counter for every write.
@addaleax addaleax requested a review from jasnell March 7, 2018 17:36
@nodejs-github-bot 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 addaleax requested a review from apapirovski March 7, 2018 17:37
@addaleax
Copy link
Member Author

addaleax commented Mar 7, 2018

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 7, 2018
@BridgeAR
Copy link
Member

@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.

@addaleax
Copy link
Member Author

@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/

@addaleax
Copy link
Member Author

addaleax commented Mar 11, 2018 •
edited
Loading

Landed in 90e70b8

@addaleax addaleax closed this Mar 11, 2018
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 11, 2018
@addaleax addaleax deleted the http2-simplify-timeout-tracking branch March 11, 2018 19:16
addaleax added a commit that referenced this pull request Mar 11, 2018
@addaleax
http2: simplify timeout tracking
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>
targos pushed a commit that referenced this pull request Mar 17, 2018
@addaleax @targos
http2: simplify timeout tracking
8e9c50f 
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>
@targos targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@addaleax @MylesBorins
http2: simplify timeout tracking
89fbbc4 
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@addaleax @MayaLekova
http2: simplify timeout tracking
33385c7 
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>
@BethGriggs
Copy link
Member

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

@kjin
Copy link
Contributor

kjin commented Oct 18, 2018

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.

kjin pushed a commit to kjin/node that referenced this pull request Oct 20, 2018
@addaleax @kjin
http2: simplify timeout tracking
c6b2a7b 
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 kjin mentioned this pull request Oct 20, 2018
Closed
2 tasks
@addaleax addaleax added the http2 Issues or PRs related to the http2 subsystem. label Oct 20, 2018
@addaleax
Copy link
Member Author

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

@addaleax
Copy link
Member Author

@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

MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@addaleax @MylesBorins
http2: simplify timeout tracking
7f34c27 
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>
@BethGriggs BethGriggs mentioned this pull request Nov 12, 2018
Merged
@beevelop beevelop mentioned this pull request Nov 20, 2018
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@apapirovski apapirovski apapirovski approved these changes

@danbev danbev danbev approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@addaleax @BridgeAR @BethGriggs @kjin @apapirovski @danbev @jasnell @cjihrig @nodejs-github-bot