socket.write() emits error synchronously when socket was closed by peer

[sam-github]

• Version: all
• Platform: all
• Subsystem: net

var net = require('net')

var s = net.createServer().listen(0).on('listening', function() {
  var port = this.address().port;
  var c = net.connect(port).on('connect', function() {
    setTimeout(function() {
      var er;
      c.on('error', function(e) {
        er = e;
        console.log('on er', e);
      });
      c.write('helo');
      console.log('after write', er);
    }, 5);
  });
});

s.on('connection', function(c) {
  c.end();
});

EDIT: forgot output:

core/node (master % u=) % ./node ../sync-error.js
on er { Error: This socket has been ended by the other party
    at Socket.writeAfterFIN [as write] (net.js:407:12)
    at Timeout._onTimeout (/home/sam/w/core/sync-error.js:12:9)
    at listOnTimeout (timers.js:324:15)
    at processTimers (timers.js:268:5) code: 'EPIPE' }
after write { Error: This socket has been ended by the other party
    at Socket.writeAfterFIN [as write] (net.js:407:12)
    at Timeout._onTimeout (/home/sam/w/core/sync-error.js:12:9)
    at listOnTimeout (timers.js:324:15)
    at processTimers (timers.js:268:5) code: 'EPIPE' }

I would expect error to be emitted in the next tick, as it normally would for a runtime error. This matters because normally code like this would work:

sock,write(...)
sock.on('error', ...)

but in the case of a TCP close by the peer, the error event will never be caught in the above.

I think the TODO in the source is referring to this:

node/lib/net.js

Line 409 in 5c2d555

// TODO: defer error events consistently everywhere, not just the cb

@mcollina what do you think? Is how net sockets are working here how a write stream is expected to work?

[mcollina]

No, let's fix this. I fear there will be some ecosystem breakage, but I would say that's ok.

[addaleax]

I would strongly prefer for the fix to be that we have as little net-specific behaviour as possible. Overriding socket.write shouldn’t be something we’re doing at all, imo.

[sam-github]

I think socket.destroy() has a similar problem, in that it synchronously emits the 'close' event.

[mcollina]

@sam-github I don't think so, see https://github.com/nodejs/node/blob/master/lib/net.js#L611. Can you reproduce?

[sam-github]

Can't repro, I'm not sure anymore what I saw when I took that note. Write sync emitting an error isn't specific to server half-close, it looks like its just generally how it works.

const assert = require('assert')
const net = require('net')

const s = net.createServer((sock) => {
  s.close()
}).listen(function() {
    const port = this.address().port
  const c = net.connect(this.address().port, () => {
    let errored
    c.once('error', () => errored=true)
    c.destroy()
    c.end('END')
    assert(!errored, 'error should be in next tick')
  })
})

[Trott]

Fixed in 9389b46

[Trott]

(Feel free to re-open if you think this should stay open until that change is in a release, or all relevant releases, or whatever.)

[huyhoangk50]

Is it just on fixed on node 12.8.0? From my side, other versions still have this problem.

[Trott]

Is it just on fixed on node 12.8.0? From my side, other versions still have this problem.

Fixed in 12.0.0 and newer. It will not be fixed in older release lines because the fix is considered a breaking change.