Node: net: Don't unlink unix socket on server.close()

[laino]

• Platform: Linux

Right now before an unix socket is closed its file is also unlinked by libuv. This behavior seems to go against the current docs, which explicitly mention that the socket would persist until unlinked (seems to imply this doesn't happen automatically).

This behavior has already been discussed multiple times in past, and every time the result
of the conversation appears to have been that it is a bad idea.

Some problems this behavior causes:

• It makes restarts with little downtime very hard to implement, since normally
  you would unlink the old socket in your new process immediately before listening to your new one.
  However, if your old process calls server.close() afterwards, it will unlink your new socket. Bummer.
• A simple workaround would be not calling server.close(), however now you lose the ability
  to wait for old connection to die before exiting, and also the ability to not accept new connections before
  you exit.

The best way to work around this problem I've found so far appears to be this safeListen:

// Edit: Replaced old workaround with a better one. Now we
// just rename the socket after creating it, so libuv can't know
// about the new path and thus can't unlink it.
const fs = require('fs');

function safeListen(httpServer, path, callback) {
    callback = callback || function(error) {
        if (error) {
            httpServer.emit('error', error);
        }
    };

    const tmpPath = path + '.' + process.pid + '.tmp';

    httpServer.listen(tmpPath, (error) => {
        if (error) {
            return callback(error);
        }

        fs.rename(tmpPath, path, (error) => {
            if (error) {
                httpServer.close(() => {});
                return callback(error);
            }

            callback(null, httpServer);
        });
    });
};

[addaleax]

/cc @gireeshpunathil

[gireeshpunathil]

@laino - reading your 2 statements together

It makes restarts with little downtime very hard to implement

and

if your old process calls server.close() afterwards, it will unlink your new socket. Bummer.

I get an impression that your use case is to start a new instance before (or while) the old one dies.
Also I guess you mean to say that it is the most common scenario.
Can you please confirm?

My take is that, I am sure that is the case - scripts that monitor process PIDs determine when a process died and when to start a fresh one.

However, I agree that if the process dies abruptly there is a chance of stale sockets and can block the automated way of restarting, but isn't it covered by deleting it before listing? Am I missing anything?

In either case (common use case or rare) /cc @nodejs/http to review the proposal

This behavior seems to go against the current docs

Agree, this was reported and is being worked upon - https://github.com/nodejs/help/issues/1080 and https://github.com/nodejs/node/pull/19471

[laino]

[...] but isn't it covered by deleting it before listing? Am I missing anything?

I don't think my initial explanation of this was very coherent. So please consider this simplified case where we start a new process, wait for it to become ready, and only then stop the old process:

1. Old: Is running and accepts connections on mySock.sock
2. New: Starts and tries to listen on the socket:
   
   
   
   • fs.unlink("mySock.sock")
   
   
   • server.listen("mySock.sock")
   
   
3. New: Is now ready and signals Old to stop
4. Old: Stops listening on the socket and waits for remaining requests to be served:
   
   
   
   • server.close()
   
   
   • libuv: fs.unlink("mySock.sock") <- Oh No!
   
   

Our Old process just unlinked our New socket!

This problem is precisely why most existing software using unix sockets don't unlink
their socket at all when they close it, instead they put one unlink(...) right before bind(...).

[Trott]

@bnoordhuis, @indutny, @nodejs/streams

[mcollina]

I think this should be very sensible to do. At least adding an option to not close the socket.

It would be great to review a PR.