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

http2: fix kUpdateTimer timer refresh #18062

Merged
merged 1 commit into from
Jan 11, 2018

Conversation

Fishrock123
Copy link
Contributor

Fixes an oversight from 93eb68e 😬

Wasn't caught by a test... @nodejs/http2 anyone have suggestions on how to write a test? Or if you want to write one and push it here that would be fine too.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes (Unfortunately)
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@Fishrock123 Fishrock123 added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. http2 Issues or PRs related to the http2 subsystem. labels Jan 9, 2018
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Jan 9, 2018
@jasnell
Copy link
Member

jasnell commented Jan 9, 2018

timeouts are definitely one area where we need more test coverage for http2. It's been on my list, just haven't been able to get to it yet.

@Fishrock123 Fishrock123 changed the title http: fix kUpdateTimer timer refresh http2: fix kUpdateTimer timer refresh Jan 9, 2018
@Fishrock123
Copy link
Contributor Author

Fixes an oversight from
93eb68e

Wasn't caught by a test.

PR-URL: nodejs#18062
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Fishrock123 Fishrock123 merged commit 0812ebd into nodejs:master Jan 11, 2018
@Fishrock123 Fishrock123 deleted the fix-93eb68e branch January 11, 2018 17:16
@evanlucas
Copy link
Contributor

This is not landing cleanly on v9.x-staging. If you would like to see this land in v9.x, can you please submit a backport PR? Thanks!

@BridgeAR
Copy link
Member

This is based on a semver-major PR. So I added the do not land labels.

@addaleax addaleax mentioned this pull request Oct 20, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants