Repository navigation
Writing to socket in Node.js 19 passes null in callback #47229
Description
Activity
- changed the title
[-]Writing to unix socket in Node.js 19 passes `null` in callback[/-][+]Writing to socket in Node.js 19 passes `null` in callback[/+]on Mar 23, 2023 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(); } ); } ) } );
FWIW in general (not just node core) it's usually best to just do
!errin error-first callbacks, as sometimes you'll encounterundefinedand other timesnull(or maybe evenfalsein less common instances), but the meaning is the same.I don't think this counts as a regression because, to the best of my knowledge, the documentation doesn't claim anywhere that the
errargument 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.
Aha, there seems to have been an issue complaining about the opposite problem :)
Which resulted in this PR:
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!
cc @targos - this needs your input
My suggestion: revert that change and declare that
err === undefinedgoing forward, now and forever.Reacted by Arman Bilge and Joram RuitenschildWhy my input? I'm not familiar with this issue.
Sorry, I meant @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.
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 (whereundefinedandnullare handled differently) and this is the first time I've encountered anullerror instead of anundefinedone. Since this happens on the happy-path it's not an easy thing to miss.Other codepaths continue to use
undefinede.g.const net = require("net") const server = net.createServer() server.listen() server.close(x => console.log(x))
$ npx node@19 test.js undefinedOther codepaths continue to use
undefinede.g.const net = require("net") const server = net.createServer() server.listen() server.close(x => console.log(x))
$ npx node@19 test.js undefinedThe
'close'event (which is what the callback passed toserver.close()is attached to) is never passed any arguments, soxshould be expected to beundefined.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 objectI've extensively wrapped the Node.js
net,tls,fs,zlib, and other APIs in Scala.js (whereundefinedandnullare handled differently) and this is the first time I've encountered anullerror instead of anundefinedone.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
netandtlssee #44290 (comment).Thanks! I guess I never encountered those due to using the
fs.promisesand zlibstream.TransformAPIs.Ok, so if
nullis the blessed way to indicate no error, then everywhere usingundefined(e.g.server.close()) should be changed?Ok, so if
nullis 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 soundefinedis 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.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=== trueor=== falsein their condition can still explicitly cast to boolean.IMHO the two paths from here are:
-
Change them all to
null(or change them all toundefined), for consistency and principle of least surprise; or -
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?
-
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 ;).
I kicked off the process of getting the TypeScript bindings changed to use
err?: Error | nullrather thanerr?: Errorto align with reality here. DefinitelyTyped/DefinitelyTyped#72657
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?
How often does it reproduce? Is there a required condition?
No response
What is the expected behavior? Why is that the expected behavior?
What do you see instead?
Additional information
No response