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

test-gzip now failing when using node 10.1 #2940

Closed
gareth-robinson opened this issue May 14, 2018 · 3 comments
Closed

test-gzip now failing when using node 10.1 #2940

gareth-robinson opened this issue May 14, 2018 · 3 comments

Comments

@gareth-robinson
Copy link

gareth-robinson commented May 14, 2018

Summary

When running the CI tests against node 10.1. a number of the tests cause deprecation warnings, but the test-gzip fails test 'do not try to pipe responses with no body'

{ Error: socket hang up
    at createHangUpError (_http_client.js:313:15)
    at Socket.socketOnEnd (_http_client.js:409:23)
    at Socket.emit (events.js:187:15)
    at endReadableNT (_stream_readable.js:1086:12)
    at process._tickCallback (internal/process/next_tick.js:63:19) code: 'ECONNRESET' }

Simplest Example to Reproduce

As above, just run CI tests against node 10.1

node_modules/.bin/taper tests/test-gzip.js

Possible Solution

The particular failing test in test-gzip creates a test server and then tries getting 105, 204 and 304 responses for it in order to test empty responses (lines 270-295 in test-gzip.js). If I comment out the '105' part of the test it passes.
The reason the 105 test fails appears to be as a result of this issue/fix in node
nodejs/node#9282
nodejs/node#18033
I think as a result of the change, Informational responses leave the response open for another response to be written later - so doing

res.writeHead(105, ...
res.end()

no longer leaves the response in the right state.

Context

Means that any pull request submitted will not get a green build.

@gareth-robinson
Copy link
Author

Looks like this is already being looked at, with this commit: 0c5db42 ("Skip status code 105 on Node > v10").
So I'll close this issue once that's merged in!

@simov
Copy link
Member

simov commented May 15, 2018

Yep, I just skipped it. There is a change in how 105 status code is handled in Node v10, but I'm not sure why exactly it was throwing an error.

I've read your comment now, thanks for the info.

@gareth-robinson
Copy link
Author

gareth-robinson commented May 15, 2018

I think if you write an informational response using res.writeHead it clears req.res
https://github.com/nodejs/node/blob/master/lib/_http_client.js#L527
but if you then call res.end after that without actually sending a real response, it hits this line
https://github.com/nodejs/node/blob/master/lib/_http_client.js#L420
and emits the hang up error.
So the error message isn't really correct - it wasn't a socket hangup, it was just ending the response without actually sending anything.

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

No branches or pull requests

2 participants