http: Bubble http parser errors up to ClientResponse or ClientRequest object instead of the underlying socket #3777

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@mscdex
mscdex commented Jul 27, 2012

Closes #3776

Would something like this also make it into v0.8 or no?

@mscdex
mscdex commented Aug 2, 2012

Any objections?

@bnoordhuis
Member

Would something like this also make it into v0.8 or no?

It's a change in behavior so no. Change LGTM but can you add a test?

@mscdex
mscdex commented Aug 6, 2012

Test added

@bnoordhuis
Member

The test fails for me:

$ out/Release/node test/simple/test-http-client-parse-error.js 
connection
connection
got error from client
got error from client

assert.js:102
  throw new assert.AssertionError({
        ^
AssertionError: false == true
    at ClientRequest.<anonymous> (/home/bnoordhuis/src/nodejs/master/test/simple/test-http-client-parse-error.js:54:16)
    at ClientRequest.EventEmitter.emit (events.js:88:17)
    at Socket.socketCloseListener (http.js:1314:9)
    at Socket.EventEmitter.emit (events.js:115:20)
    at Socket._destroy.destroyed (net.js:356:10)
    at process.startup.processNextTick.process._tickCallback (node.js:315:13)
    at process.startup.processMakeCallback.process._makeCallback (node.js:243:15)
@mscdex
mscdex commented Aug 6, 2012

@bnoordhuis how about now?

@bnoordhuis bnoordhuis commented on an outdated diff Aug 6, 2012
test/simple/test-http-client-parse-error.js
-
- req.on('error', function(e) {
- console.log('got error from client');
- srv.close();
- assert.ok(e.message.indexOf('Parse Error') >= 0);
- assert.equal(e.code, 'HPE_INVALID_CONSTANT');
- parseError = true;
- });
+ for (var i = 0; i < 2; i++) {
+ http.request({
+ host: '127.0.0.1',
+ port: common.PORT,
+ method: 'GET',
+ path: '/'
+ }).on('error', function(e) {
+ if (e.message.indexOf('Parse Error') > -1) {
@bnoordhuis
bnoordhuis Aug 6, 2012 Member

This check is no good. Serialize the requests so you know when to expect the parse error or start two servers, one on common.PORT and the other on common.PORT + 1, so the requests can be made in parallel.

@mscdex
mscdex commented Aug 7, 2012

Squashed and test fixed again.

EDIT: I still say this is a bug fix since there was already a test for parser errors emitted on the request object and so I think this fix should also go into v0.8

@mscdex mscdex referenced this pull request in request/request Aug 10, 2012
Merged

Allow parser errors to bubble up to request #293

@bnoordhuis
Member

Landed in c78678b, thanks. I'll let @isaacs decide if he wants to back-port it to v0.8.

@bnoordhuis bnoordhuis closed this Aug 24, 2012
@mscdex
mscdex commented Dec 6, 2012

@isaacs Any chance to backport this to v0.8?

@isaacs isaacs commented on the diff Dec 7, 2012
lib/http.js
@@ -1367,7 +1367,9 @@ function socketOnData(d, start, end) {
if (ret instanceof Error) {
debug('parse error');
freeParser(parser, req);
- socket.destroy(ret);
@isaacs
isaacs Dec 7, 2012

This seems fine, but doesn't socket.destroy(er) trigger socket.emit('error', er) which in turn triggers req.emit('error', er)? (If not, then I think that's a bug. What happens to socket errors?)

@mscdex
mscdex Dec 8, 2012

I haven't looked at this in quite some time, but IIRC before this patch (on 0.8) either you had to listen on req.connection's 'error' event to catch at least some parser errors or you were not able to catch/listen for them at all.

@isaacs
isaacs commented Dec 7, 2012

Backporting to 0.8 is fine.

@mscdex @bnoordhuis What do we do with socket errors now? Shouldn't the old code trigger an error on the req?

@bnoordhuis
Member

Shouldn't the old code trigger an error on the req?

In v0.8? It emits a 'clientError' event on the http.Server object.

@bnoordhuis
Member

Back-ported in 827b2a9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment