restify / clients

HttpClient, StringClient, and JsonClient extracted from restify
MIT License
57 stars 34 forks source link

Do not mix flowing and non-flowing mode for streams #177

Closed misterdjules closed 6 years ago

misterdjules commented 6 years ago

Moved from https://github.com/restify/node-restify/issues/1674.

Feature Request

Use Case

@hekike fixed support for Node.js >= v10.x in https://github.com/restify/clients/pull/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.

misterdjules commented 6 years ago

Landed with https://github.com/restify/clients/commit/4872e813d705a80f8259181f3c3111fa552863ae, thank you @michaelnisi 🙏