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: use session not socket timeout, tests #15188

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@apapirovski
Member

apapirovski commented Sep 4, 2017

This fixes #15158. Two changes included:

  • there's no longer a default server socket timeout, it's now replaced by a default session timeout
  • _unrefActive was being called on the handler rather than the actual session

Marked as WIP as I think we need to investigate whether socket timeout is necessary anywhere and also because this needs a lot more tests, ideally.

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

http2, test

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 5, 2017

Member

Also something I just noticed, this earlier PR should probably be revisited in light of this #15106 — I'm not sure if all of it is relevant anymore?

Member

apapirovski commented Sep 5, 2017

Also something I just noticed, this earlier PR should probably be revisited in light of this #15106 — I'm not sure if all of it is relevant anymore?

@apapirovski apapirovski changed the title from [WIP] http2: use session not socket timeout, tests to http2: use session not socket timeout, tests Sep 5, 2017

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 5, 2017

Member

@jasnell, @mcollina this should be ready to review. Ideally we would have more tests around this behaviour but it does fix the bug and it does work in a way that makes sense.

Member

apapirovski commented Sep 5, 2017

@jasnell, @mcollina this should be ready to review. Ideally we would have more tests around this behaviour but it does fix the bug and it does work in a way that makes sense.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

Can you please check if this affect performance anyhow?

Assuming it does not cause a regression.. LGTM

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 6, 2017

Member

Will test later today but I would be surprised if it doesn't affect performance at least a little bit. For one, _unrefActive was being called on something that wasn't a timer and two, the socket timeout was never _unrefActive either.

Member

apapirovski commented Sep 6, 2017

Will test later today but I would be surprised if it doesn't affect performance at least a little bit. For one, _unrefActive was being called on something that wasn't a timer and two, the socket timeout was never _unrefActive either.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Sep 6, 2017

Member

I think the failures on Mac OS X are unrelated.

Let us know about the performance impact.

@jasnell what do you think?

Member

mcollina commented Sep 6, 2017

I think the failures on Mac OS X are unrelated.

Let us know about the performance impact.

@jasnell what do you think?

@mcollina mcollina requested a review from jasnell Sep 6, 2017

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 6, 2017

Member

Will need to figure out how to get h2load built on a Mac so this might take me a little while. Will update when I make it work.

Member

apapirovski commented Sep 6, 2017

Will need to figure out how to get h2load built on a Mac so this might take me a little while. Will update when I make it work.

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 6, 2017

Member

@mcollina It's not too bad after all.

 http2/headers.js nheaders=0 n=1000        -1.62 %        *** 7.214983e-05
 http2/headers.js nheaders=10 n=1000       -1.29 %            6.246195e-02
 http2/headers.js nheaders=100 n=1000      -0.90 %            2.357148e-01
 http2/headers.js nheaders=1000 n=1000      0.70 %            4.645484e-01

Not too surprising that there's a tiny bit of a hit since before this PR the timeouts were completely bugged and weren't doing anything (basically no-op). Same benchmark with server.timeout = 0; yields this:

 http2/headers.js nheaders=0 n=1000         2.41 %            0.1543180
 http2/headers.js nheaders=10 n=1000        1.42 %            0.3769546
 http2/headers.js nheaders=100 n=1000       0.82 %            0.5944166
 http2/headers.js nheaders=1000 n=1000      0.19 %            0.9174064

Edit: This is after 40 runs on each node version btw.

Member

apapirovski commented Sep 6, 2017

@mcollina It's not too bad after all.

 http2/headers.js nheaders=0 n=1000        -1.62 %        *** 7.214983e-05
 http2/headers.js nheaders=10 n=1000       -1.29 %            6.246195e-02
 http2/headers.js nheaders=100 n=1000      -0.90 %            2.357148e-01
 http2/headers.js nheaders=1000 n=1000      0.70 %            4.645484e-01

Not too surprising that there's a tiny bit of a hit since before this PR the timeouts were completely bugged and weren't doing anything (basically no-op). Same benchmark with server.timeout = 0; yields this:

 http2/headers.js nheaders=0 n=1000         2.41 %            0.1543180
 http2/headers.js nheaders=10 n=1000        1.42 %            0.3769546
 http2/headers.js nheaders=100 n=1000       0.82 %            0.5944166
 http2/headers.js nheaders=1000 n=1000      0.19 %            0.9174064

Edit: This is after 40 runs on each node version btw.

@jasnell

jasnell approved these changes Sep 6, 2017

LGTM with green CI

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 7, 2017

Member

There was too much red in the last CI run, trying again: https://ci.nodejs.org/job/node-test-commit/12214/

Member

jasnell commented Sep 7, 2017

There was too much red in the last CI run, trying again: https://ci.nodejs.org/job/node-test-commit/12214/

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 7, 2017

Member

CI is definitely not green on this one: https://ci.nodejs.org/job/node-test-commit-linuxone/8427/nodes=rhel72-s390x/console

@apapirovski ... can you take a look?

btw, I really appreciate that you've jumped in on these. PR's from new contributors make me super happy :-)

Member

jasnell commented Sep 7, 2017

CI is definitely not green on this one: https://ci.nodejs.org/job/node-test-commit-linuxone/8427/nodes=rhel72-s390x/console

@apapirovski ... can you take a look?

btw, I really appreciate that you've jumped in on these. PR's from new contributors make me super happy :-)

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 7, 2017

Member

I think the test might be flaky because of the low timeout. Will look into it.

Member

apapirovski commented Sep 7, 2017

I think the test might be flaky because of the low timeout. Will look into it.

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 7, 2017

Member

@jasnell can we run the CI again? I adjusted the timing to be less strict to match some of the other timeout tests. At 20ms the margin for error was too small.

Member

apapirovski commented Sep 7, 2017

@jasnell can we run the CI again? I adjusted the timing to be less strict to match some of the other timeout tests. At 20ms the margin for error was too small.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 7, 2017

Member

Worked this time. Nice! Thanks for the reviews @mcollina & @jasnell.

Member

apapirovski commented Sep 7, 2017

Worked this time. Nice! Thanks for the reviews @mcollina & @jasnell.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Sep 7, 2017

Member

This does not land cleanly on master, can you please rebase and squash?

Member

mcollina commented Sep 7, 2017

This does not land cleanly on master, can you please rebase and squash?

http2: use session not socket timeout, tests
Change default timeout to be tracked on the session instead
of the socket, as nghttp2 manages the socket and we would
need to maintain two sets of timeouts for similar purpose.
Also fixes session setTimeout to work as it wasn't getting
_unrefActive correctly (was called on the handle).

Fixes: #15158
@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 7, 2017

Member

All done.

Member

apapirovski commented Sep 7, 2017

All done.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Sep 7, 2017

Member

Landed as 46133b5

Member

mcollina commented Sep 7, 2017

Landed as 46133b5

@mcollina mcollina closed this Sep 7, 2017

mcollina added a commit that referenced this pull request Sep 7, 2017

http2: use session not socket timeout, tests
Change default timeout to be tracked on the session instead
of the socket, as nghttp2 manages the socket and we would
need to maintain two sets of timeouts for similar purpose.
Also fixes session setTimeout to work as it wasn't getting
_unrefActive correctly (was called on the handle).

Fixes: #15158
PR-URL: #15188
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@apapirovski apapirovski deleted the apapirovski:patch-http2-session-timeout branch Sep 7, 2017

MylesBorins added a commit that referenced this pull request Sep 10, 2017

http2: use session not socket timeout, tests
Change default timeout to be tracked on the session instead
of the socket, as nghttp2 manages the socket and we would
need to maintain two sets of timeouts for similar purpose.
Also fixes session setTimeout to work as it wasn't getting
_unrefActive correctly (was called on the handle).

Fixes: #15158
PR-URL: #15188
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 11, 2017

http2: use session not socket timeout, tests
Change default timeout to be tracked on the session instead
of the socket, as nghttp2 manages the socket and we would
need to maintain two sets of timeouts for similar purpose.
Also fixes session setTimeout to work as it wasn't getting
_unrefActive correctly (was called on the handle).

Fixes: #15158
PR-URL: #15188
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@mcollina mcollina referenced this pull request Sep 11, 2017

Closed

test: fix flaky test-http2-session-timeout #15328

2 of 2 tasks complete

MylesBorins added a commit that referenced this pull request Sep 12, 2017

http2: use session not socket timeout, tests
Change default timeout to be tracked on the session instead
of the socket, as nghttp2 manages the socket and we would
need to maintain two sets of timeouts for similar purpose.
Also fixes session setTimeout to work as it wasn't getting
_unrefActive correctly (was called on the handle).

Fixes: #15158
PR-URL: #15188
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax added a commit to addaleax/node that referenced this pull request Sep 13, 2017

http2: use session not socket timeout, tests
Change default timeout to be tracked on the session instead
of the socket, as nghttp2 manages the socket and we would
need to maintain two sets of timeouts for similar purpose.
Also fixes session setTimeout to work as it wasn't getting
_unrefActive correctly (was called on the handle).

Fixes: nodejs#15158
PR-URL: nodejs#15188
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment