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

net,tls,test: remove writeQueueSize and fix bufferSize #15006

Closed
wants to merge 1 commit into from

Conversation

micooz
Copy link

@micooz micooz commented Aug 24, 2017

When use TLSSocket, bufferSize is always +1, this affects users who
rely on this property to make judgments. This commit removed legacy
code and corrected the calculation of bufferSize.

Fixes: #15005

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

When use TLSSocket, bufferSize is always +1, this affects users who
rely on this property to make judgments. This commit removed legacy
code and corrected the calculation of bufferSize.

Fixes: nodejs#15005
@nodejs-github-bot nodejs-github-bot added net Issues and PRs related to the net subsystem. tls Issues and PRs related to the tls subsystem. labels Aug 24, 2017
@mscdex
Copy link
Contributor

mscdex commented Aug 24, 2017

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

hmm... would this end up being a semver-major?? It's really not clear.

@indutny
Copy link
Member

indutny commented Aug 24, 2017

I'm afraid this may not be a correct change. write_queue_size_string is used in src/ folder and this value is updated from C++ callbacks.

@BridgeAR
Copy link
Member

@indutny do you have a suggestion how to handle this properly?

@BridgeAR
Copy link
Member

Ping @indutny

@BridgeAR
Copy link
Member

Ping @nodejs/crypto @mcollina

@mcollina
Copy link
Member

I cannot help but I trust @indutny.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 24, 2017

@indutny would you be so kind and give a hint how the correct approach would look like?

@indutny
Copy link
Member

indutny commented Sep 24, 2017

@BridgeAR sorry for delay. Technically it might be possible to keep the number of pending bytes in that property. Perhaps it should be approached in a way of fixing it rather than removing it?

@BridgeAR
Copy link
Member

As far as I see it the proper fix for the bufferSize would be to just removing the writeQueueSize from the return value but keep it in place otherwise as it was before. Do you agree @indutny?

@indutny
Copy link
Member

indutny commented Sep 24, 2017

Hm... on a second thought this might be a right way to do it indeed. Still the check if net.js is pretty important and should be left as it is.

@@ -767,7 +767,7 @@ Socket.prototype._writeGeneric = function(writev, data, encoding, cb) {

// If it was entirely flushed, we can write some more right now.
// However, if more is left in the queue, then wait until that clears.
if (req.async && this._handle.writeQueueSize !== 0)
Copy link
Member

Choose a reason for hiding this comment

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

This condition should be left as it is.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2017

Ping @micooz

apapirovski added a commit to apapirovski/node that referenced this pull request Oct 18, 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.

Fixes: nodejs#15005
Refs: nodejs#15006
apapirovski added a commit that referenced this pull request Oct 21, 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: #15791
Fixes: #15005
Refs: #15006
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@apapirovski
Copy link
Member

Thank you for trying to improve Node @micooz — since a fix for this landed in #15791 I'm going to close this PR but hope to see you around & contributing to Node!

MylesBorins pushed a commit that referenced this pull request Oct 23, 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: #15791
Fixes: #15005
Refs: #15006
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
apapirovski added a commit to apapirovski/node that referenced this pull request Oct 25, 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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request 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/node#15791
Fixes: nodejs/node#15005
Refs: nodejs/node#15006
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 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/node#15791
Fixes: nodejs/node#15005
Refs: nodejs/node#15006
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
apapirovski added a commit to apapirovski/node that referenced this pull request Dec 12, 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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants