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.

Sign up for GitHub

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

Jump to bottom

tls: fix writeQueueSize prop, long write timeouts #15791

Closed
wants to merge 3 commits into from
Closed

tls: fix writeQueueSize prop, long write timeouts #15791

wants to merge 3 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Oct 6, 2017 •
edited
Loading

This PR addresses a long-standing issue of _handle.writeQueueSize in TLS not representing the actual write queue (#15005), as well as the fact that long-lasting writes on both net & tls (http & https) will currently timeout if a socket timeout is set (#15082). The latter has been reported as being in relation to the keep alive timeout but in reality it's just a broad issue that is the most obvious with keep-alive since it's much shorter at 5s (vs 120s).

  • Make writeQueueSize represent the actual size of the write queue within the TLS socket.
  • Add tls test to confirm that bufferSize works as expected.

Second commit:

  • Add getWriteQueueSize on TLSWrap which updates and returns queue size (net & tls).
  • Make _onTimeout check whether an active write is ongoing and if so, call _unrefTimer rather than emitting a timeout event.
  • Add http & https test that checks whether long-lasting (but active) writes timeout or can finish writing as expected.

Haven't worked in C++ in a very, very long time so would definitely appreciate any and all feedback there.

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)

http, https, net, test, tls

@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 Oct 6, 2017
@mscdex
Copy link
Contributor

mscdex commented Oct 6, 2017

/cc @indutny

@apapirovski apapirovski force-pushed the patch-http-keep-alive-timeout branch from bdc43c5 to fdf7a79 Compare October 7, 2017 17:00
@apapirovski
Copy link
Member Author

Just a minor update to use fixtures.readKey instead. (Indirectly benefiting and learning from code-and-learn 👍)

/cc @mcollina

@mcollina
Copy link
Member

mcollina commented Oct 7, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/10452/

@mcollina
Copy link
Member

mcollina commented Oct 7, 2017

Is updating this state have a performance impact? Have you tried to run our benchmarks? I think we can run them on CI if you do not have the spare CPU cycles (they take a long time).

@apapirovski
Copy link
Member Author

apapirovski commented Oct 7, 2017 via email •
edited
Loading

@apapirovski
Copy link
Member Author

apapirovski commented Oct 7, 2017 via email

@apapirovski
Copy link
Member Author

I've updated the tests to, hopefully, be less flaky. Would appreciate if we could get another CI started. Thanks!

@joyeecheung
Copy link
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/10495/

@apapirovski
Copy link
Member Author

apapirovski commented Oct 8, 2017 •
edited
Loading

Well, this might take a while. If anyone has any ideas on making these less timeout dependant (while still testing the timeout bug), I'm all ears.

@apapirovski apapirovski force-pushed the patch-http-keep-alive-timeout branch from 06f4d9f to 875872d Compare October 8, 2017 14:51
@apapirovski
Copy link
Member Author

apapirovski commented Oct 8, 2017 •
edited
Loading

Ok, I think this should work... ? I can run a stress test on the test on my local system now and it doesn't fail (before only 50% succeeded). Since it's still in sequential (and should stay there), I'm hoping that'll make it not fail on the slower systems.

@joyeecheung
Copy link
Member

Another CI: https://ci.nodejs.org/job/node-test-pull-request/10498/

@apapirovski
Copy link
Member Author

Well, this seems to succeed everywhere but SmartOS so I guess an improvement... Anyone have any suggestions by any chance? Going to try setting up a SmartOS box to test on...

@apapirovski apapirovski force-pushed the patch-http-keep-alive-timeout branch from 875872d to 8969906 Compare October 8, 2017 20:20
@apapirovski
Copy link
Member Author

apapirovski commented Oct 8, 2017 •
edited
Loading

Ok, one last try. If this doesn't fix SmartOS then there's a deeper issue that might not have anything to do with the test itself. I can run this for an hour on my system (96 instances at a time) without any failures (previous version would fail like 2.5% of the time).

(And master still fails this test 100% of the time.)

@mcollina
Copy link
Member

mcollina commented Oct 9, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/10527/

@apapirovski apapirovski force-pushed the patch-http-keep-alive-timeout branch from 8969906 to daf7931 Compare October 9, 2017 11:47
@apapirovski
Copy link
Member Author

apapirovski commented Oct 9, 2017 •
edited
Loading

Oops, I accidentally deleted a few characters before committing the http version of the test so it was testing with readSize of 3333 instead of 333333. No wonder it failed. Could we run the CI just on SmartOS (and OS X)?

If that doesn't work, I have one other potential direction to take this test...

@mcollina
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10565/

@apapirovski
Copy link
Member Author

Thanks for starting it @mcollina. No luck this time either :( No clue if there's a bug in http on SmartOS related to writeQueueSize or if it's just the test. Almost done setting up a SmartOS box so I guess I'll find out...

@apapirovski
Copy link
Member Author

Having a SmartOS box really made this easy to fix. Should be good for another CI run now.

@mcollina
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10584/

@apapirovski
Copy link
Member Author

Thanks! Looks like it worked this time (the SmartOS failure is unrelated, it's in parallel/test-benchmark-string_decoder). Now to get some reviews... :)

ping @indutny since you know the most about the TLS code.

@apapirovski apapirovski force-pushed the patch-http-keep-alive-timeout branch from e00c69c to caa179d Compare October 12, 2017 14:02
apapirovski added a commit to apapirovski/node that referenced this pull request Oct 25, 2017
@apapirovski
net: fix timeouts during long writes
96f7df8 
Add updateWriteQueueSize which updates and returns queue size
(net & tls). Make _onTimeout check whether an active write
is ongoing and if so, call _unrefTimer rather than emitting
a timeout event.

Add http & https test that checks whether long-lasting (but
active) writes timeout or can finish writing as expected.

PR-URL: nodejs#15791
Fixes: nodejs#15082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@joyeecheung joyeecheung mentioned this pull request Oct 25, 2017
Closed
@apapirovski apapirovski mentioned this pull request Oct 25, 2017
Closed
3 tasks
cjihrig pushed a commit that referenced this pull request Oct 25, 2017
@apapirovski @cjihrig
net: fix timeout with null handle
cecbb59 
This commit handles the case where _onTimeout is called with a
null handle.

Refs: #15791
Fixes: #16484
PR-URL: #16489
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
cjihrig pushed a commit that referenced this pull request Oct 25, 2017
@apapirovski @cjihrig
net: fix timeout with null handle
bf02780 
This commit handles the case where _onTimeout is called with a
null handle.

Refs: #15791
Fixes: #16484
PR-URL: #16489
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
apapirovski added a commit to apapirovski/node that referenced this pull request Oct 25, 2017
@apapirovski
net: fix timeout with null handle
cd44ee1 
This commit handles the case where _onTimeout is called with a
null handle.

Refs: nodejs#15791
Fixes: nodejs#16484
PR-URL: nodejs#16489
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@apapirovski @addaleax
tls: properly track writeQueueSize during writes
2635efd 
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

PR-URL: nodejs/node#15791
Fixes: nodejs/node#15005
Refs: nodejs/node#15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@apapirovski @addaleax
net: fix timeouts during long writes
b3699eb 
Add updateWriteQueueSize which updates and returns queue size
(net & tls). Make _onTimeout check whether an active write
is ongoing and if so, call _unrefTimer rather than emitting
a timeout event.

Add http & https test that checks whether long-lasting (but
active) writes timeout or can finish writing as expected.

PR-URL: nodejs/node#15791
Fixes: nodejs/node#15082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@apapirovski @addaleax
net: fix timeout with null handle
99ade12 
This commit handles the case where _onTimeout is called with a
null handle.

Refs: nodejs/node#15791
Fixes: nodejs/node#16484
PR-URL: nodejs/node#16489
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@apapirovski @addaleax
tls: properly track writeQueueSize during writes
f47398d 
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

PR-URL: nodejs/node#15791
Fixes: nodejs/node#15005
Refs: nodejs/node#15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@apapirovski @addaleax
net: fix timeouts during long writes
adfe2b6 
Add updateWriteQueueSize which updates and returns queue size
(net & tls). Make _onTimeout check whether an active write
is ongoing and if so, call _unrefTimer rather than emitting
a timeout event.

Add http & https test that checks whether long-lasting (but
active) writes timeout or can finish writing as expected.

PR-URL: nodejs/node#15791
Fixes: nodejs/node#15082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@apapirovski @addaleax
net: fix timeout with null handle
47813f7 
This commit handles the case where _onTimeout is called with a
null handle.

Refs: nodejs/node#15791
Fixes: nodejs/node#16484
PR-URL: nodejs/node#16489
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
apapirovski added a commit to apapirovski/node that referenced this pull request Dec 12, 2017
@apapirovski
tls: properly track writeQueueSize during writes
7533f59 
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

PR-URL: nodejs#15791
Fixes: nodejs#15005
Refs: nodejs#15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
apapirovski added a commit to apapirovski/node that referenced this pull request Dec 12, 2017
@apapirovski
net: fix timeouts during long writes
3b5ac77 
Add updateWriteQueueSize which updates and returns queue size
(net & tls). Make _onTimeout check whether an active write
is ongoing and if so, call _unrefTimer rather than emitting
a timeout event.

Add http & https test that checks whether long-lasting (but
active) writes timeout or can finish writing as expected.

PR-URL: nodejs#15791
Fixes: nodejs#15082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
apapirovski added a commit to apapirovski/node that referenced this pull request Dec 12, 2017
@apapirovski
net: fix timeout with null handle
920f2ff 
This commit handles the case where _onTimeout is called with a
null handle.

Refs: nodejs#15791
Fixes: nodejs#16484
PR-URL: nodejs#16489
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2017
@apapirovski @MylesBorins
tls: properly track writeQueueSize during writes
f588355 
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

Backport-PR-URL: #16420
PR-URL: #15791
Fixes: #15005
Refs: #15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2017
@apapirovski @MylesBorins
net: fix timeouts during long writes
ab9c527 
Add updateWriteQueueSize which updates and returns queue size
(net & tls). Make _onTimeout check whether an active write
is ongoing and if so, call _unrefTimer rather than emitting
a timeout event.

Add http & https test that checks whether long-lasting (but
active) writes timeout or can finish writing as expected.

Backport-PR-URL: #16420
PR-URL: #15791
Fixes: #15082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2017
@apapirovski @MylesBorins
net: fix timeout with null handle
15d855d 
This commit handles the case where _onTimeout is called with a
null handle.

Backport-PR-URL: #16420
Refs: #15791
Fixes: #16484
PR-URL: #16489
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 2, 2018
@apapirovski @MylesBorins
tls: properly track writeQueueSize during writes
521dc25 
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

Backport-PR-URL: #16420
PR-URL: #15791
Fixes: #15005
Refs: #15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 2, 2018
@apapirovski @MylesBorins
net: fix timeouts during long writes
a301c1a 
Add updateWriteQueueSize which updates and returns queue size
(net & tls). Make _onTimeout check whether an active write
is ongoing and if so, call _unrefTimer rather than emitting
a timeout event.

Add http & https test that checks whether long-lasting (but
active) writes timeout or can finish writing as expected.

Backport-PR-URL: #16420
PR-URL: #15791
Fixes: #15082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 2, 2018
@apapirovski @MylesBorins
net: fix timeout with null handle
a09e2fd 
This commit handles the case where _onTimeout is called with a
null handle.

Backport-PR-URL: #16420
Refs: #15791
Fixes: #16484
PR-URL: #16489
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@bnoordhuis bnoordhuis bnoordhuis left review comments

@mcollina mcollina mcollina approved these changes

@indutny indutny indutny approved these changes

@refack refack refack approved these changes

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
9 participants
@apapirovski @mscdex @mcollina @joyeecheung @refack @indutny @bnoordhuis @addaleax @nodejs-github-bot