http: Stream#write with an empty buffer does not call callback

[RubenVerborgh]

When writing an empty buffer, Stream#write does not call the callback: https://github.com/nodejs/node/blob/v10.7.0/lib/_http_outgoing.js#L608

Is this intended? If so, this should probably be mentioned explicitly in the documentation.
If not, we might want to fix this.

[RubenVerborgh]

Note: it only hangs on empty buffers, not empty strings.

[addaleax]

I think this has come up before, in some form? It would be a breaking change but probably worth it.

/cc @nodejs/http

[amitzur]

I reported the original issue to axios. I now wonder how this code does work (or in other words how to reproduce with a minimal example):

process.stdout.write(Buffer.from(''), err => {
  console.log('done!', err);
});

output:

done! undefined

[RubenVerborgh]

@amitzur The code that returns early with an empty buffer is specific to HTTP, not to writable streams in general. Your example doesn't use HTTP, so that's why it works.

[amitzur]

ahh, right right.
It hangs also on empty strings. Which makes sense from the code at https://github.com/nodejs/node/blob/v10.7.0/lib/_http_outgoing.js#L608 but it doesn't explain why follow-redirects doesn't hang on empty strings.

For example:

require('http')
  .request({
    hostname: 'www.google.com',
  })
  .write('', err => {
    console.log('done', err);
  });

[mcollina]

Fixed in 413a7e1.

[RubenVerborgh]

It does not explicitly mention that the callback will not be called, however. So this doesn't address my original issue.

Also, please note that

it only hangs on empty buffers, not empty strings.

so this is a problem as well.

[RubenVerborgh]

@mcollina Do I create a new issue?

[mcollina]

It does not explicitly mention that the callback will not be called, however. So this doesn't address my original issue.

@RubenVerborgh I think it does 413a7e1#diff-69e1ac0b5bfc06e74f2c1ab7b062c6afR753 - When I read that, I assume nothing means nothing, including not calling the callback.

Would you like to send a PR with the text that would be ok for you?

it only hangs on empty buffers, not empty strings.

Can you please upload a snippet that show this? The check and the logic is identical, so I don't see how it's possible that the two call will behave differently Buffer.from('').length and ''.length both return 0.

[RubenVerborgh]

Would you like to send a PR with the text that would be ok for you?

Proposal in #22461.

That said, spelling things out explicitly begs the question whether it is a good design decision to have this exception for empty buffers. Basically, every call to write with a callback will need to be surrounded by an if statement if the caller did not create the buffer itself.

[RubenVerborgh]

Can you please upload a snippet that show this?

I thought I had evidence for this in a test suite of a project, but I was mistaken. The behavior is identical indeed, as expected from the code.

[mcollina]

@RubenVerborgh thanks for checking!

[addaleax]

We still want to change the behaviour so that the callback does get called, right? I.e. #22461 is not a full resolution of this issue, right?

[mcollina]

That matches the behavior of streams unfortunately, it’s not just http. There is no guarantee that the write callback is called.

[RubenVerborgh]

There is no guarantee that the write callback is called.

In that case, we might want to update the documentation at https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback.

[mcollina]

That doc is correct in the letter, but possibly unclear to the reader. The callback is called when data is flushed, however it’s _not_ called if it’s not flushed (as an example in case of errors unrelated to the write). If we can frame it better, would you mind sending a PR our way? Thanks! Il giorno mer 22 ago 2018 alle 23:22 Ruben Verborgh < notifications@github.com> ha scritto:

…

There is no guarantee that the write callback is called. In that case, we might want to update the documentation at https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback . — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#22066 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL44lNmBkqjiUWu7A3cp2bKVpE9lNMks5uTcuxgaJpZM4Vq_Vs> .

[RubenVerborgh]

In #22493 (comment), @mcollina confirmed that Stream#write always calls the callback, even with empty chunks. So OutgoingMessage#write breaks that contract.

Can we fix OutgoingMessage#write to adhere to the contract (semver-major)?

[mcollina]

Can we fix OutgoingMessage#write to adhere to the contract (semver-major)?

Yes.