[v10.x] tls: group chunks into TLS segments #28904
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Correct docs to clarify that behaviour, and fix a race condition in test-http2-large-write-destroy.js. Fixes: nodejs#27863 PR-URL: nodejs#27891 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#28903 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
TLSWrap::DoWrite() now concatenates data chunks and makes a single call to SSL_write(). Grouping data into a single segment: - reduces network overhead: by factors of even 2 or 3 in usages like `http2` or `form-data` - improves security: segment lengths can reveal lots of info, i.e. with `form-data`, how many fields are sent and the approximate length of every individual field and its headers - reduces encryption overhead: a quick benchmark showed a ~30% CPU time decrease for an extreme case, see nodejs#27573 (comment) Fixes: nodejs#27573 PR-URL: nodejs#27861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot
added
c++
|
Tests are passing. /ping @nodejs/backporters for reviews/landing |
|
@nodejs/crypto |
bnoordhuis
approved these changes
Aug 3, 2019
There was a problem hiding this comment.
Thanks, LGTM.
Instead I've defined it as
vector<char>which has almost identical API; are these changes okay for backports?
Ideally the delta between release lines is as small as possible. If it's not a gargantuan task to back-port the AllocatedBuffer API, then that's strongly preferred.
|
Then I'll try to also backport #26207 and see if it works. |
|
Thanks, at least you tried. I'm good with landing this PR as is. |
BethGriggs
approved these changes
Oct 7, 2019
BethGriggs
pushed a commit
that referenced
this pull request
Oct 7, 2019
Correct docs to clarify that behaviour, and fix a race condition in test-http2-large-write-destroy.js. Fixes: #27863 Backport-PR-URL: #28904 PR-URL: #27891 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs
pushed a commit
that referenced
this pull request
Oct 7, 2019
TLSWrap::DoWrite() now concatenates data chunks and makes a single call to SSL_write(). Grouping data into a single segment: - reduces network overhead: by factors of even 2 or 3 in usages like `http2` or `form-data` - improves security: segment lengths can reveal lots of info, i.e. with `form-data`, how many fields are sent and the approximate length of every individual field and its headers - reduces encryption overhead: a quick benchmark showed a ~30% CPU time decrease for an extreme case, see #27573 (comment) Fixes: #27573 Backport-PR-URL: #28904 PR-URL: #27861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
|
Landed in 7f48519...fe58bca |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #27861 (and also #27891 and #28903 to fix tests).
The original code uses
AllocatedBuffer, so we would need to also backport #26207 (specifically 6c257cd). Instead I've defined it asvector<char>which has almost identical API; are these changes okay for backports?