-
Notifications
You must be signed in to change notification settings - Fork 30k
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
net: fix bufferSize #34088
Conversation
@nodejs/net |
lib/net.js
Outdated
req = writeGeneric(this, data, encoding, cb); | ||
if (req.async) | ||
this[kLastWriteQueueSize] = req.bytes; | ||
writeGeneric(this, data, encoding, cb); |
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.
writableLength
won't get subtracted until the write actually completes. Not sure what the purpose of kLastWriteQueueSize
is here.
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 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.
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.
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
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.
Ok, can you check when this was originally added? The original commit might shed some light.
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.
Perhaps at that time writableLength
wasn't working like this? I don't know.
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.
unclear...
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.
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.
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.
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?)
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.
Seems like handle.writeQueueSize
is not zero before invoking the callback. Which is kind of confusing.
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.
@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);
}
This code seemed a bit suspicious to me as well, but anytime I changed something, CI broke, so … I’ll kick off CI 😄 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
So from what I can tell Furthermore, this causes a problem since |
@ronag Maybe the problem is a discrepancy between 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);
}
|
This comment has been minimized.
This comment has been minimized.
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
Are you talking about a |
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'
} |
This comment has been minimized.
This comment has been minimized.
@addaleax Are we in agreement on |
This comment has been minimized.
This comment has been minimized.
Maybe something to do with tls handshake or something? |
@addaleax Regarding |
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 |
Landed in 8f4b4f2...0edeeec |
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), orvcbuild test
(Windows) passes