socket.write callback error argument is undefined

[prettydiff]

Version

18.6.0

Platform

Microsoft Windows NT 10.0.19044.0 x64

Subsystem

none

What steps will reproduce the bug?

1. Create a new socket using the net library.

import { Socket } from "net";
import { connect } from "tls";
const socket:Socket = connect({
    host: config.ip,
    port: config.port,
    rejectUnauthorized: false
})

2. Write to the socket and provide a callback.

socket.write("hello", "utf8", function (writeError) {
    console.log(writeError);
});

It does not appear the writeError argument on the callback is populating correctly. By default the value should be null if no error messaging is passed or of type NodeJS.ErrnoException if an error is passed. Instead I am getting undefined.

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 null or an object of type NodeJS.ErrnoException.

What do you see instead?

undefined.

Additional information

No response

[lpinca]

This is not unique to the socket.write() callback. I think that most callbacks are called with an undefined error argument instead of null.

[prettydiff]

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.

[lpinca]

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.ClientRequest and http.ServerResponse write callbacks are called with undefined.

net.Socket inherits from stream.Writable so fixing this for writable.write() might also fix it for socket.write().

[lpinca]

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.

[prettydiff]

It appears there is an actual valid defect occurring here more than whether the error comes back as null or undefined.

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.

[mk-pmb]

It looks like your code ignores the return value from socket.write which would tell you whether the kernel had to buffer stuff and thus you should wait for the drain event. Might this solve your problem?

[prettydiff]

@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.

[mk-pmb]

@prettydiff I'd omit the === true though. At best it's useless, but probably it's a waste of performance. In the worst case it might even cause bugs.

[everhardt]

This change is breaking quite some downstream code. For example, the callback of the send function of the ws package should now be checked for undefined and null. And @types/ws has 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?

[armanbilge]

Yes, this change broke our code as well. See:

• Writing to socket in Node.js 19 passes null in callback #47229