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: fix bufferSize #34088

Closed
wants to merge 2 commits into from
Closed

net: fix bufferSize #34088

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 27, 2020

bufferSize should only look at writableLength otherwise it will always show more than what is actually pending.

I'm not familiar with this code but it just looks wrong/suspicious to me.

Tests seem to pass with changes.

Maybe someone more familiar could take a look at determine whether this seems reasonable and what a good test could be?

Refs: #34078

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

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jun 27, 2020
@ronag
Copy link
Member Author

ronag commented Jun 27, 2020

@nodejs/net

@ronag ronag requested review from jasnell and lpinca June 27, 2020 17:41
lib/net.js Outdated
req = writeGeneric(this, data, encoding, cb);
if (req.async)
this[kLastWriteQueueSize] = req.bytes;
writeGeneric(this, data, encoding, cb);
Copy link
Member Author

@ronag ronag Jun 27, 2020

Choose a reason for hiding this comment

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

writableLength won't get subtracted until the write actually completes. Not sure what the purpose of kLastWriteQueueSize is here.

Copy link
Member

@lpinca lpinca Jun 27, 2020

Choose a reason for hiding this comment

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

This is my understanding: assume there is nothing buffered so writableLength is zero and there is a write in progress, socket.bufferSize should report the number of bytes that are currently being written and not zero.

Copy link
Member Author

@ronag ronag Jun 27, 2020

Choose a reason for hiding this comment

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

I don't think writableLength is zero here, it becomes non zero as soon as you do write() and won't become zero again unless all writes have finished

should report the number of bytes that are currently being writte

writableLength does exactly that as far as I can tell

Copy link
Member

Choose a reason for hiding this comment

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

Ok, can you check when this was originally added? The original commit might shed some light.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps at that time writableLength wasn't working like this? I don't know.

Copy link
Member Author

Choose a reason for hiding this comment

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

unclear...

Copy link
Member Author

Choose a reason for hiding this comment

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

The only explanation I can think of is if the callback is called before the write has actually completed... which would be a bit weird.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I don’t think that that’s what happens (but I guess it could be checked easily enough through verifying that the write queue size is 0 when we get the callback from libuv?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like handle.writeQueueSize is not zero before invoking the callback. Which is kind of confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax the following assertion will fail when running the test suite:

diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js
index fdf540a47e..4c7049bf51 100644
--- a/lib/internal/stream_base_commons.js
+++ b/lib/internal/stream_base_commons.js
@@ -30,6 +30,7 @@ const {
 } = require('internal/timers');
 const { isUint8Array } = require('internal/util/types');
 const { clearTimeout } = require('timers');
+const assert = require('assert');
 
 const kMaybeDestroy = Symbol('kMaybeDestroy');
 const kUpdateTimer = Symbol('kUpdateTimer');
@@ -104,6 +105,8 @@ function onWriteComplete(status) {
   stream[kUpdateTimer]();
   stream[kAfterAsyncWrite](this);
 
+  assert(this.handle.writeQueueSize === 0);
+
   if (typeof this.callback === 'function')
     this.callback(null);
 }

@addaleax
Copy link
Member

This code seemed a bit suspicious to me as well, but anytime I changed something, CI broke, so … I’ll kick off CI 😄

@nodejs-github-bot
Copy link
Collaborator

@ronag

This comment has been minimized.

@ronag

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@ronag

This comment has been minimized.

@ronag ronag added the http Issues or PRs related to the http subsystem. label Jun 27, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Jun 27, 2020

So from what I can tell writeQueueSize is not zero when the callback is invoked, i.e. the handle has some kind of internal buffer that is not always flushed before the callback is invoked. Which kind of makes write(chunk, callback) lie in regards to whether the write was successful or not.

Furthermore, this causes a problem since writableLength also includes parts of writeQueueSize. Basically what we want is bufferSize = writableLength + writeQueueSize - dispatchedLength where dispatchedLength is the current size of whatever was sent to writeGeneric.

@addaleax
Copy link
Member

@ronag Maybe the problem is a discrepancy between handle.writeQueueSize and the write_queue_size property as reported by libuv?

This diff doesn’t result in any obvious crashes:

--- a/src/stream_wrap.cc
+++ b/src/stream_wrap.cc
@@ -400,6 +400,7 @@ void LibuvStreamWrap::AfterUvWrite(uv_write_t* req, int status) {
   CHECK_NOT_NULL(req_wrap);
   HandleScope scope(req_wrap->env()->isolate());
   Context::Scope context_scope(req_wrap->env()->context());
+  CHECK_EQ(static_cast<LibuvStreamWrap*>(req_wrap->stream())->stream()->write_queue_size, 0);
   req_wrap->Done(status);
 }
 

@ronag

This comment has been minimized.

@addaleax
Copy link
Member

What is writeQueueSize and where does it come from?

My understanding is that it is the size of the currently active write. This is not necessarily equivalent to the size of the last scheduled write, because sometimes a large write is only partially written and then the rest of it. libuv communicated that through write_queue_size. We use this for timeout tracking – if write_queue_size has decreased since the last time, it means that data has been written in the background by libuv, without any action from Node.js, hence we can infer that the socket is active and not timing out.

I seem to get a writeQueueSize of 80 even when nothing has been written to the socket.

Are you talking about a net.Socket? Do you have a repro? This sounds very wrong. 😄

@ronag
Copy link
Member Author

ronag commented Jun 27, 2020

@addaleax

diff --git a/test/parallel/test-tls-streamwrap-buffersize.js b/test/parallel/test-tls-streamwrap-buffersize.js
index 984cc51e50..6ec028c621 100644
--- a/test/parallel/test-tls-streamwrap-buffersize.js
+++ b/test/parallel/test-tls-streamwrap-buffersize.js
@@ -52,6 +52,7 @@ const net = require('net');
         socket,
         rejectUnauthorized: false,
       }, common.mustCall(() => {
+        assert.strictEqual(client._handle.writeQueueSize, 0);
         assert.strictEqual(client.bufferSize, 0);
 
         for (let i = 1; i < iter; i++) {
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

80 !== 0

    at TLSSocket.<anonymous> (/Users/ronagy/GitHub/nxtedition/node/test/parallel/test-tls-streamwrap-buffersize.js:55:16)
    at TLSSocket.<anonymous> (/Users/ronagy/GitHub/nxtedition/node/test/common/index.js:365:15)
    at Object.onceWrapper (events.js:420:28)
    at TLSSocket.emit (events.js:314:20)
    at TLSSocket.onConnectSecure (_tls_wrap.js:1523:10)
    at TLSSocket.emit (events.js:314:20)
    at TLSSocket._finishInit (_tls_wrap.js:933:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:707:12)
    at DuplexSocket.ondata (internal/js_stream_socket.js:77:22)
    at DuplexSocket.emit (events.js:314:20) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 80,
  expected: 0,
  operator: 'strictEqual'
}

@ronag

This comment has been minimized.

@ronag
Copy link
Member Author

ronag commented Jun 27, 2020

@addaleax Are we in agreement on bufferSize === writableLength should be true or am I missing something?

@ronag

This comment has been minimized.

@ronag
Copy link
Member Author

ronag commented Jun 27, 2020

Are you talking about a net.Socket? Do you have a repro? This sounds very wrong. 😄

Maybe something to do with tls handshake or something?

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Jun 27, 2020

@addaleax Regarding writeQueueSize, I'm curious where exactly it is implemented?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 28, 2020

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 29, 2020
@ronag
Copy link
Member Author

ronag commented Jun 29, 2020

are deprecations always semver-major?

@richardlau
Copy link
Member

are deprecations always semver-major?

Generally yes (for non-doc-only deprecations) unless the TSC make an exception: https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#deprecations

lpinca added a commit to websockets/ws that referenced this pull request Jun 29, 2020
ronag added a commit that referenced this pull request Jun 30, 2020
bufferSize should only look at writableLength otherwise it will
always show more than what is actually pending.

PR-URL: #34088
Refs: #34078
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
ronag added a commit that referenced this pull request Jun 30, 2020
PR-URL: #34088
Refs: #34078
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@ronag
Copy link
Member Author

ronag commented Jun 30, 2020

Landed in 8f4b4f2...0edeeec

@ronag ronag closed this Jun 30, 2020
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
bufferSize should only look at writableLength otherwise it will
always show more than what is actually pending.

PR-URL: #34088
Refs: #34078
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
PR-URL: #34088
Refs: #34078
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
bufferSize should only look at writableLength otherwise it will
always show more than what is actually pending.

PR-URL: #34088
Refs: #34078
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
PR-URL: #34088
Refs: #34078
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
bufferSize should only look at writableLength otherwise it will
always show more than what is actually pending.

PR-URL: #34088
Refs: #34078
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. deprecations Issues and PRs related to deprecations. http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants