node 16.x does not raise error on write when server closes connection

[tlbdk]

Node version and platform

$ node -v
v16.0.0
$ uname -a
Darwin Troelss-Mac-mini.local 20.4.0 Darwin Kernel Version 20.4.0: Thu Apr 22 21:46:41 PDT 2021; root:xnu-7195.101.2~1/RELEASE_ARM64_T8101 arm64

What steps will reproduce the bug?

let os = require('os')
let net = require('net')
let crypto = require('crypto')

let socketPath = `${os.tmpdir()}/${crypto.randomBytes(12).toString('hex')}.sock`

let server = net.createServer(socket => {
    let buffer = Buffer.alloc(0)
    socket.on('data', chunk => {
        if(chunk.toString('utf8') === 'destroy') {
            console.log('server destroy')
            socket.destroy()
        }       
    })
})
server.on('listening', () => {
    // client closes
    const socket1 = net.connect({ path: socketPath })
    socket1.on('error', (e) => {
        console.log('socket1:' + e.toString())
    })
    socket1.end()
    socket1.write('hello')

    // server closes
    const socket2 = net.connect({ path: socketPath })
    socket2.on('error', (e) => {
        console.log('socket2:' + e.toString())
    })
    socket2.write('destroy')
    setTimeout(() => {
        socket2.write()
    }, 1000)
})
server.listen(socketPath)

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

Every time.

What is the expected behavior?

Node 12.x:

$ /opt/homebrew/Cellar/node@12/12.22.1_1/bin/node socket-errors.js
socket1:Error [ERR_STREAM_WRITE_AFTER_END]: write after end
server destroy
socket2:Error: This socket has been ended by the other party

Node 14.x:

$ /opt/homebrew/Cellar/node@14/14.17.0/bin/node socket-errors.js 
socket1:Error [ERR_STREAM_WRITE_AFTER_END]: write after end
server destroy
socket2:Error: This socket has been ended by the other party

What do you see instead?

Node 16.x:

$ /opt/homebrew/Cellar/node/16.0.0_1/bin/node socket-errors.js 
socket1:Error [ERR_STREAM_WRITE_AFTER_END]: write after end
server destroy

Additional information

[targos]

@nodejs/streams ?

[ronag]

@nodejs/net

[mcollina]

This behavior is correct and wanted, and it is the combination of a few PRs that landed in v15.

What is happening is:

1. the server destroy() the connection.
2. the client emit 'close', no more errors should be emitted.
3. after 1 second, the client try sending some data. No error is emitted, but it is passed to the .write() callback.

In your client example, you are not waiting 1 second before sending the .write() message and the socket is not closed yet.

Here is a complete example:

let os = require('os')
let net = require('net')
let crypto = require('crypto')

let socketPath = `${os.tmpdir()}/${crypto.randomBytes(12).toString('hex')}.sock`

let server = net.createServer(socket => {
    let buffer = Buffer.alloc(0)
    socket.on('data', chunk => {
        console.log('chunk', chunk.toString())
        if(chunk.toString('utf8') === 'destroy') {
            console.log('server destroy')
            socket.destroy()
        }
    })
})
server.on('listening', () => {
    // client closes
    const socket1 = net.connect({ path: socketPath })
    socket1.on('error', (e) => {
        console.log('socket1:' + e.toString())
    })
    socket1.end()
    setTimeout(function () {
      socket1.write('hello', function (err) {
        console.log('hello written', err)
      })
    }, 1000)

    // server closes
    const socket2 = net.connect({ path: socketPath })
    socket2.on('error', (e) => {
        console.log('socket2:' + e.toString())
    })
    socket2.write('destroy')
    setTimeout(() => {
        socket2.write('something', function (err) {
          console.log('something written', err)
        })
    }, 1000)
    socket2.on('end', () => {
      console.log('socket2 ended')
    })
    socket2.on('close', () => {
      console.log('socket2 closed')
    })
})
server.listen(socketPath)

[tlbdk]

@mcollina I understand the reasoning and it is more consistent, the documentation could be a bit more clear that the callback for socket.write callback will returns an error as this quite important to avoid silently loosing the write.

[mcollina]

@mcollina I understand the reasoning and it is more consistent, the documentation could be a bit more clear that the callback for socket.write callback will returns an error as this quite important to avoid silently loosing the write.

Agreed.

https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback looks quite confusing in this regard.

[mcollina]

Would you like to send a pull request?

[inidaname]

Hello @mcollina does this still need a PR, I would love to take a shot at this

[mcollina]

Yes it does, it's a doc update.

[inidaname]

could you point me to the doc file. thank you

[mcollina]

could you point me to the doc file. thank you

https://github.com/nodejs/node/blob/master/doc/api/stream.md

[inidaname]

Hello @mcollina
I will love to know what is the expected result for this correction. I could not draft a better alternative to the current docs.
Thank you

[mcollina]

@tlbdk what did you find unclear in the current docs? What would you change?

[Pulkit3234]

@mcollina sir, I am new to open source, can you please guide me on what this issue is about, so that I can try to contribute, Thanks!

[mcollina]

@mcollina sir, I am new to open source, can you please guide me on what this issue is about, so that I can try to contribute, Thanks!

Check out https://www.nodetodo.org/

This issue is probably too hard for a newcomer, I removed the "good first issue" tag.

[simoneb]

The doc change requires somebody who knows exactly how streams behave in 16.x. Adding a clarification about the behavior discussed in this thread would still not clarify the existing documentation, which states:

If an error occurs, the callback may or may not be called with the error as its first argument. To reliably detect write errors, add a listener for the 'error' event.

Based on what I understand from this thread we are saying that it's expected that once the end event is fired, then no more events will be fired (including error), meaning that if a write() happens after the end event, the error event is not fired but instead the write callback is invoked with an error object.

Simply adding this explanation would still not clarify under which circumstances:

the callback may or may not be called with the error as its first argument

If somebody could clarify what are those circumstances fixing the docs will be easier.

[mcollina]

Based on what I understand from this thread we are saying that it's expected that once the end event is fired, then no more events will be fired (including error), meaning that if a write() happens after the end event, the error event is not fired but instead the write callback is invoked with an error object.

After close is emitted there will be no more stream-related events. The write callback is always invoked with an Error object if one exist. After 'close' is emitted, there won't be any stream-wide errors anymore.

[simoneb]

@mcollina apologies for being pedantic but this still doesn't clarify it fully, also suggesting that getting this right is not straightforward.

Quoting you:

The write callback is always invoked with an Error object if one exist. After 'close' is emitted, there won't be any stream-wide errors anymore.

The first sentence somewhat contradicts the second. If the callback is always called with an error object if one exists, how can there not be any stream-wide errors anymore after close is emitted?

Perhaps the second sentence means more specifically that close is the last ever event you'll ever see on a stream but the write callback will still be called with an error, if one exists, in all circumstances?

If that is the case, do we agree that this contradicts the current docs which say:

If an error occurs, the callback may or may not be called with the error as its first argument

and which need to be fixed by removing that sentence and replacing it with a rephrase of what you wrote above?

Also, the docs say right after the previous quote:

To reliably detect write errors, add a listener for the 'error' event.

That would then not be completely accurate, because in order to reliably detect errors you would have both to listen to the error event and handle the error in the write callback handler, in case a write happens after the stream has already been closed, meaning that no errors will be emitted by the stream anymore.

[simoneb]

After clarifying with @mcollina I created #38959 to change the docs in order to reflect the current behavior.