Do not mix flowing and non-flowing mode for streams
misterdjules opened this issue · 1 comments
Moved from restify/node-restify#1674.
- Used appropriate template for the issue type
- Searched both open and closed issues for duplicates of this issue
- Title adequately and concisely reflects the feature or the bug
Feature Request
Use Case
@hekike fixed support for Node.js >= v10.x in #175. That work highlighted the fact that some streams are used both in flowing and non-flowing mode in the current implementation.
This has historically been a common source of bugs and confusion. I believe making sure that streams use either flowing or non-flowing mode APIs would make the code more robust.
For instance, with the changes from the PR mentioned above, if emitResult was later changed to emit the 'result' event asynchronously (using e.g. setImmediate or process.nextTick), it would re-introduce the same broken behavior (the client would not read the stream).
Example API
The StringClient class should be reading from the stream using .read() instead of .on('data') to make changes such as the one mentioned above not introduce any regression.
We should also audit other streams implementations in the code base and make sure they're not using a mix of flowing and non-flowing mode APIs.
Are you willing and able to implement this?
Yes.
Landed with 4872e81, thank you @michaelnisi