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

tls: renegotiate should take care of its own state #25997

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Feb 7, 2019

In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: #14239
See: b1909d3a70f9

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

In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: nodejs#14239
See: nodejs@b1909d3a70f9
@sam-github
Copy link
Contributor Author

I don't offer any justifiation for this change, its streams black magic. I'm curious to see if it passes CI.

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Feb 7, 2019
@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

Any thoughts on this? @jasnell @cjihrig @addaleax @mcollina

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks for the ping :)

Btw, in the long run, it might make sense to move TLSWrap to a HTTP2-style model, where we check at the end of methods whether we have data to send or not, and if so, schedule a write.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Cautious LGTM

@danbev
Copy link
Contributor

danbev commented Feb 13, 2019

Landed in 666beb0.

@danbev danbev closed this Feb 13, 2019
danbev pushed a commit that referenced this pull request Feb 13, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: #14239
See: b1909d3a70f9

PR-URL: #25997
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 13, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: #14239
See: b1909d3a70f9

PR-URL: #25997
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
@sam-github sam-github deleted the remove-suspicious-zero-length-write branch March 4, 2019 22:06
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request May 28, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: nodejs#14239
See: nodejs@b1909d3a70f9

PR-URL: nodejs#25997
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs pushed a commit that referenced this pull request Jun 7, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: #14239
See: b1909d3a70f9

Backport-PR-URL: #27938
PR-URL: #25997
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants