GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — net,tls,test: remove writeQueueSize and fix bufferSize by micooz · Pull Request #15006 · nodejs/node

Conversation

@micooz

@micooz micooz commented Aug 24, 2017

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@micooz
net,tls,test: remove writeQueueSize and fix bufferSize
035bf88 
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
@micooz micooz mentioned this pull request Aug 24, 2017
TLSSocket.bufferSize is always +1 before and after write() #15005
Closed
@mscdex

mscdex commented Aug 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

/cc @indutny ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Aug 24, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Aug 24, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Aug 31, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Sep 13, 2017

Copy link
Copy Markdown
Member

Ping @indutny
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell requested a review from mcollina September 15, 2017 22:48
@BridgeAR

BridgeAR commented Sep 19, 2017

Copy link
Copy Markdown
Member

Ping @nodejs/crypto @mcollina
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 19, 2017

Copy link
Copy Markdown
Member

I cannot help but I trust @indutny.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Sep 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Sep 24, 2017

Copy link
Copy Markdown
Member

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Sep 24, 2017

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Sep 24, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
indutny
indutny reviewed Sep 24, 2017
View reviewed changes
Comment thread lib/net.js

// 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)

@indutny indutny Sep 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@BridgeAR

BridgeAR commented Oct 2, 2017

Copy link
Copy Markdown
Member

Ping @micooz
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
apapirovski added a commit to apapirovski/node that referenced this pull request Oct 18, 2017
@apapirovski
tls: properly track writeQueueSize during writes
4b94697 
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
@apapirovski
tls: properly track writeQueueSize during writes
aaf2a1c 
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

apapirovski commented Oct 21, 2017

Copy link
Copy Markdown
Contributor

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@apapirovski apapirovski closed this Oct 21, 2017
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@apapirovski @MylesBorins
tls: properly track writeQueueSize during writes
eeef06a 
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
@apapirovski
tls: properly track writeQueueSize during writes
008843d 
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
@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 <[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
@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 <[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
@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 <[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
@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 <[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
@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 <[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

Reviewers

@mcollina mcollina Awaiting requested review from mcollina
1 more reviewer
@indutny indutny indutny left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

net Issues and PRs related to the net subsystem. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@micooz @mscdex @jasnell @indutny @BridgeAR @mcollina @apapirovski @nodejs-github-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.