stream: sync writes don't need drain

[ronag]

Minor fix and performance improvement. Synchronous writes do not require drain.

This can be a significant improvement especially for transform streams that do not perform any async work.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

NOTE TO SELF: Sync transform streams should have a no buffer option (i.e. hwm === 0).

[Trott]

@nodejs/streams

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24969/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24973/

[mcollina]

Good work! I agree that this is going to make things faster (however we will need benchmarks), but this is likely going to break users in many ways.

We can see this in the tests, where a bunch of nextTick had to be added. Do you think it would be possible to make this backward compatible?

[ronag]

@mcollina: I'm unsure how bad of a breaking change this actually is. The test "wrongly" assume that 'drain' will be emitted. I can't imagine much logic out in the wild that depends on that behaviour for anything but resume().

That being said. The only "fully" backwards compatible way I can think of is to do this as a hack for pipe(). Where this optimization is only applied when using pipe()? Or do you have any other ideas?

[mcollina]

Then I think we should not implement this. We might decide to bring https://www.npmjs.com/package/syncthrough into core, as it solves the same problem in a non-breaking way.

[ronag]

@mcollina: Fair enough... feel free to close