Writing to socket in Node.js 19 passes null in callback

[armanbilge]

Version

v19.8.1

Platform

Linux armanbilge-sandbox-g0l1nfjuvbe 5.15.0-47-generic #51-Ubuntu SMP Thu Aug 11 07:51:15 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

const net = require("net");

const server = net.createServer();

server.listen(
  "tmp.sock",
  function() {
    const socket = new net.Socket();
    socket.connect(
      "tmp.sock",
      function() {
        socket.write(
          "hello world",
          function(x) {
            console.log(x);
            socket.destroy();
            server.close();
          }
        );
      }
    )
  }
);

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

$ npx node@18 unix.js 
undefined

What do you see instead?

$ npx node@19 unix.js 
null

Additional information

No response

[armanbilge]

Clarification: this is not specific to unix sockets.

const net = require("net");

const server = net.createServer();

server.listen(
  "localhost:8123",
  function() {
    const socket = new net.Socket();
    socket.connect(
      "localhost:8123",
      function() {
        socket.write(
          "hello world",
          function(x) {
            console.log(x);
            socket.destroy();
            server.close();
          }
        );
      }
    )
  }
);

[mscdex]

FWIW in general (not just node core) it's usually best to just do !err in error-first callbacks, as sometimes you'll encounter undefined and other times null (or maybe even false in less common instances), but the meaning is the same.

[bnoordhuis]

I don't think this counts as a regression because, to the best of my knowledge, the documentation doesn't claim anywhere that the err argument is always undefined in case of success, only that it's false-y. Node has never been super strict in that respect.

If the change from undefined to null is a problem for you, then you're welcome to open a pull request to change it back.

[armanbilge]

Aha, there seems to have been an issue complaining about the opposite problem :)

• socket.write callback error argument is undefined #44290

Which resulted in this PR:

• stream: use null for the error argument #44312

I'd understand if this was changed across the codebase for a consistent API/UX, but it seems like it was changed only for Writable :) It looks like this change broke other libraries and type definitions as well.

If the change from undefined to null is a problem for you, then you're welcome to open a pull request to change it back.

@bnoordhuis with this additional context, do you still feel like that? Thanks!

[bnoordhuis]

cc @targos - this needs your input

My suggestion: revert that change and declare that err === undefined going forward, now and forever.

[targos]

Why my input? I'm not familiar with this issue.

[bnoordhuis]

Sorry, I meant @lpinca.

[lpinca]

I would personally not revert it. It was done for consistency (see #44290 (comment)) and in major release because it was known that it could break something. Anyway, I have no strong opinion. I'm fine either way.

[armanbilge]

It was done for consistency

That's interesting, because it's completely counter to my own experience :) the comment says:

In the last 5 years I have been using TypeScript with Node and explicitly typing everything and providing extensive error handling for all events.

I've extensively wrapped the Node.js net, tls, fs, zlib, and other APIs in Scala.js (where undefined and null are handled differently) and this is the first time I've encountered a null error instead of an undefined one. Since this happens on the happy-path it's not an easy thing to miss.

Other codepaths continue to use undefined e.g.

const net = require("net")
const server = net.createServer()
server.listen()
server.close(x => console.log(x))

$ npx node@19 test.js 
undefined

[mscdex]

Other codepaths continue to use undefined e.g.

const net = require("net")
const server = net.createServer()
server.listen()
server.close(x => console.log(x))

$ npx node@19 test.js 
undefined

The 'close' event (which is what the callback passed to server.close() is attached to) is never passed any arguments, so x should be expected to be undefined.

[armanbilge]

Thanks for the input. That's not how it's working currently :)

const net = require("net")
const server = net.createServer()
server.close(x => console.log(typeof(x)))

$ npx node@19 test.js 
object

[lpinca]

I've extensively wrapped the Node.js net, tls, fs, zlib, and other APIs in Scala.js (where undefined and null are handled differently) and this is the first time I've encountered a null error instead of an undefined one.

Here are some examples:

const fs = require('fs');

fs.open('test.txt', 'w', function (err, fd) {
  console.log(err); // null
  fs.write(fd, 'foo', function (err) {
    console.log(err); // null
    fs.close(fd, function (err) {
      console.log(err); // null
    });
  });
});

const zlib = require('zlib');

zlib.deflate(Buffer.from('aaaaaaaaaa'), function (err, buf) {
  console.log(err); // null
  zlib.inflate(buf, function (err) {
    console.log(err); // null
  });
});

For net and tls see #44290 (comment).

[armanbilge]

Thanks! I guess I never encountered those due to using the fs.promises and zlib stream.Transform APIs.

Ok, so if null is the blessed way to indicate no error, then everywhere using undefined (e.g. server.close()) should be changed?

[lpinca]

Ok, so if null is the blessed way to indicate no error, then everywhere using undefined (e.g. server.close()) should be changed?

I think so. The server.close() case is weird because the callback is added as a listener for the 'close' event. The 'close' event is emitted without arguments so undefined is expected, but the callback is also called with an error if the server is not listening. I don't know if there are other cases like this.

[mk-pmb]

The blessed way to deal with it, in cases where there really even is a potential error, is to apply the robustness principle. Usually you'll use that first argument in a condition expression anyway, either bare or negated. If you use it in a way where undefined and null have a meaningfull difference, I suspect there might be a bigger problem in your code.
Anyone who really loves to have === true or === false in their condition can still explicitly cast to boolean.

[armanbilge]

IMHO the two paths from here are:

1. Change them all to null (or change them all to undefined), for consistency and principle of least surprise; or

2. Don't change anything anymore, because it's unnecessary breakage for non-robust codebases, and users correctly applying robustness won't be surprised either way.

Personally I'm fine with (2) under the assumption that #44312 would be the last time this sort of change squeaks in 😁 but if these changes are going to keep happening, wouldn't it be better to decide on a consistent API and rip off the bandaid?

[everhardt]

If you use it in a way where undefined and null have a meaningfull difference, I suspect there might be a bigger problem in your code.

When you use Typescript (depending on your tsconfig settings) this is typically not true and you do have to check for both. And nowadays, one might suspect a bigger problem in your code when you're not using Typescript ;).