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

[v6.x backport] tls: fix writeQueueSize prop, long write timeouts #16420

Closed

Conversation

apapirovski
Copy link
Member

This is a backport of #15791 as requested.

/cc @MylesBorins

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)

net, tls, test

@apapirovski apapirovski added net Issues and PRs related to the net subsystem. tls Issues and PRs related to the tls subsystem. labels Oct 23, 2017
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Oct 23, 2017
@apapirovski apapirovski changed the base branch from master to v6.x-staging October 23, 2017 22:41
@apapirovski apapirovski removed build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Oct 23, 2017
@MylesBorins
Copy link
Contributor

@apapirovski a bit confused... doesn't seem like I requested this backport in the issue. Did we cross wires?

@apapirovski
Copy link
Member Author

apapirovski commented Oct 23, 2017

@MylesBorins You did in our conversation earlier. The name of the issue might just be a tad confusing. See #13391

Either way, even if you hadn't, this should be back-ported given that it's a serious bug. But if you don't want this back-ported for v4.x let me know as I'm almost done.

@MylesBorins
Copy link
Contributor

@apapirovski AHHHHHH... thanks ☺️

This is slated to land in 8.8.0. Which means that we can likely land it in the following 6.x release, and a future 4.x maintenance release (assuming we get signoff on the backport)

@apapirovski apapirovski 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. v6.x labels Oct 23, 2017
@apapirovski
Copy link
Member Author

@MylesBorins MylesBorins changed the base branch from v6.x-staging to v6.x October 25, 2017 08:08
@MylesBorins MylesBorins changed the base branch from v6.x to v6.x-staging October 25, 2017 08:08
@apapirovski apapirovski force-pushed the backport-15791-to-v6.x branch from b6ed49b to 7feac5c Compare October 25, 2017 12:35
@apapirovski
Copy link
Member Author

CI is all green (failure unrelated) and I've rebased.

@apapirovski apapirovski added the blocked PRs that are blocked by other issues or PRs. label Oct 25, 2017
@apapirovski
Copy link
Member Author

This is blocked because of #16484 — working on a PR which will then need to be applied here.

@apapirovski apapirovski removed the blocked PRs that are blocked by other issues or PRs. label Oct 25, 2017
@apapirovski
Copy link
Member Author

Last commit is technically from a different PR but this shouldn't land without it.

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

@mcollina
Copy link
Member

I think this should wait for some more time before backporting (if at all).

@mcollina mcollina added the baking-for-lts PRs that need to wait before landing in a LTS release. label Oct 26, 2017
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>
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>
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>
@apapirovski apapirovski force-pushed the backport-15791-to-v6.x branch from 67ca82f to 920f2ff Compare December 12, 2017 21:54
@MylesBorins
Copy link
Contributor

@mcollina is this ready to land?

@mcollina
Copy link
Member

@MylesBorins no. This PR introduced #16484 which was fixed in #16489.
If they are backported, they are backported together.
What's the process for this? I'd be inclined to have two commits in this same PR.

@apapirovski
Copy link
Member Author

@mcollina the commit from that second PR is in here

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

MylesBorins pushed a commit that referenced this pull request Dec 20, 2017
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
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
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
Copy link
Contributor

landed in 612baca...15d855d

@apapirovski apapirovski deleted the backport-15791-to-v6.x branch January 2, 2018 16:26
MylesBorins pushed a commit that referenced this pull request Jan 2, 2018
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
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
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 removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. net Issues and PRs related to the net subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants