Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(HttpClient): turn res stream to readable in HttpClient #175

Merged
merged 4 commits into from Jun 7, 2018

Conversation

hekike
Copy link
Member

@hekike hekike commented Jun 6, 2018

Turn res stream readable immediately to make it possible to consume later in inherited clients.

@hekike hekike requested a review from DonutEspresso June 6, 2018 18:20
@coveralls
Copy link

coveralls commented Jun 6, 2018

Coverage Status

Coverage remained the same at 87.024% when pulling 3231c45 on fix/node-10-res-readable into 1edd6da on master.

@@ -188,6 +188,8 @@ function rawRequest(opts, cb) {

var requestTime = new Date().getTime();
req = proto.request(opts, function onResponse(res) {
res.resume();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why this is needed?

@misterdjules
Copy link
Contributor

Great catch @hekike!

To give a bit more details: it seems the regression was introduced by nodejs/node@cf5f986, which effectively makes adding a 'readable' listener stay in non-flowing mode even after a 'data' event listener is added to the same stream.

That change was released with Node.js v10.0.0. Before that change, adding a 'data' event listener to a stream would always make it "flow". It's a bit unfortunate that it doesn't seem to be listed anywhere in the list of backward incompatible changes.

Long term, ideally we would not mix both streaming modes if possible, and we would use read() instead of .on('data'. but that would potentially be a bit involved, and so for the near term your fix looks good to me.

@@ -223,6 +219,11 @@ function rawRequest(opts, cb) {
});

emitResult((err || null), req, res);

// This has to be after res.on('data') in Node >= v10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It could be useful to add a bit more info about why that is necessary (maybe add the link to the PR in the nodejs repo?).

@misterdjules
Copy link
Contributor

It's a bit unfortunate that it doesn't seem to be listed anywhere in the list of backward incompatible changes.

My apologies, it is listed, since all major changes are listed in the changelog (look for SEMVER-MAJOR and readable).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants