Repository navigation
socket.write callback error argument is undefined #44290
Description
Activity
This is not unique to the
socket.write()callback. I think that most callbacks are called with anundefinederrorargument instead ofnull.I remember in the early days of Node that was absolutely true.
In the last 5 years I have been using TypeScript with Node and explicitly typing everything and providing extensive error handling for all events. In the past 5 years this is the first time I have seen the error argument come back undefined. I suspect this is an oversight due to low visibility as I also suspect less people open sockets directly from net/tls libraries as opposed to http/https/http2.
I suspect this is an oversight due to low visibility as I also suspect less people open sockets directly from net/tls libraries as opposed to http/https/http2.
http.ClientRequestandhttp.ServerResponsewrite callbacks are called withundefined.net.Socketinherits fromstream.Writableso fixing this forwritable.write()might also fix it forsocket.write().diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index 1054631ddf..fff9260b95 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -497,7 +497,7 @@ function afterWrite(stream, state, count, cb) { while (count-- > 0) { state.pendingcb--; - cb(); + cb(null); } if (state.destroyed) {
This seems to fix the issue and all tests pass. Feel free to open a PR. Though, i'm not sure about the semverness of such change.
- added a commit that references this issue
on Aug 20, 2022 It appears there is an actual valid defect occurring here more than whether the error comes back as
nullorundefined.Since I have posted this issue I have noticed that test automation is failing half way through my test cases on socket.write. I have narrowed down that failure to a change that looks like this:
In the following code samples:
- data is a buffer combination of a websocket frame header and a data payload that may or may not be fragments
- finish is a flag indicating whether the current data to write is the final fragment of a message
- pop is a function that pops a data out of a queue I wrote and provide recursion if the queue is not empty
prior logic
socket.write(data); if (finish === true) { socket.status = "open"; pop(true, socket); }
new logic
const callback = function terminal_server_transmission_transmitWs_queue_send_writeFrame_callback(writeError:NodeJS.ErrnoException):void { if (writeError === null || writeError === undefined) { if (finish === true) { socket.status = "open"; pop(true, socket); } } else { console.log(error); } }; socket.write(data, "utf8", callback);
I added the callback logic to ensure there are no collisions when writing data to the socket too rapidly. On the browser side, for example, if you write to a socket too rapidly such that socket.write is fired before the previous write method finishes writing to the socket and clearing the buffer the prior write is abandoned. The Node write method executes faster than the browser equivalent, my data payloads are typically small, and my hardware is fast enough to compensate for the low risk of collisions that the risk of collision is low enough that I have not noticed it yet on the Node side in my application, but the risk is there none the less so I added callback logic.
After a certain number of writes to a socket too rapidly the callback begins to fire very slowly, from sub millisecond to 4-9 seconds, and then 14 seconds, and then a complete failure to return at all. As a result persistent sockets making use of a callback appear to crash without closing the socket. I am achieving a 100% reproduction rate on this in my application.
It looks like your code ignores the return value from
socket.writewhich would tell you whether the kernel had to buffer stuff and thus you should wait for thedrainevent. Might this solve your problem?@mk-pmb Yes, that is the magic answer. I am using the following logic:
if (socket.write(data) === true) { callback(); } else { socket.once("drain", callback); }
Counter-intuitively, that is somehow dramatically faster and more stable than not using a callback at all.
- added a commit that references this issue
on Aug 23, 2022 @prettydiff I'd omit the
=== truethough. At best it's useless, but probably it's a waste of performance. In the worst case it might even cause bugs.This change is breaking quite some downstream code. For example, the callback of the send function of the
wspackage should now be checked forundefinedandnull. And@types/wshas become incorrect because of this. That's just one package, I suspect there will be many more. Are you sure it is a good idea to do this?Yes, this change broke our code as well. See:
Version
18.6.0
Platform
Microsoft Windows NT 10.0.19044.0 x64
Subsystem
none
What steps will reproduce the bug?
It does not appear the
writeErrorargument on the callback is populating correctly. By default the value should benullif no error messaging is passed or of typeNodeJS.ErrnoExceptionif an error is passed. Instead I am gettingundefined.I don't mind writing the solution and submitting a pull request, but I need some guidance. I believe the write method is defined at https://github.com/nodejs/node/blob/main/lib/net.js#L875-L914 but I cannot see any execution or definition of the callback there. The problem could also be lower in the streams library or higher in the tls library, so I am not sure how to get started debugging and writing a solution. I suspect its just a missing feature.
How often does it reproduce? Is there a required condition?
100% reproducible. No special conditions required.
What is the expected behavior?
Callback argument value is populated as either
nullor an object of typeNodeJS.ErrnoException.What do you see instead?
undefined.Additional information
No response