src: unconsume stream fix by Kasher · Pull Request #11015 · nodejs/node · GitHub

When emitting a connection event on a httpServer, the function connectionListener is called.
Then, a new parser is created, and consume method is called on the
socket's externalStream. However, if this stream was already consumed and unconsumed,
the process crashes with a cpp assert from the Consume method in
stream_base.h. This commit makes sure that no SIGABRT will be raised
and the process will stay alive (after emitting the socket).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

src

Issue: #11017

R= @indutny probably?

I think we'd rather want to handle it from JS. We should mark stream as previously consumed and fallback to slow .on('data') case.

Does this make sense?

@indutny - Thanks for your response.
May I ask why is it better to handle this from JS?
I guess it would add some state on the stream itself, and it may be easily misused in the future (if someone uses stream in other scenarios)...... I believe that when calling to "unconsume" and resetting the stream's callbacks (in node_http_parser.cc), it should reset the stream's entire state, and make it ready to be re-consumed.
But, again, I'm not too familiar with the considerations or difficulties you had while writing this code, so please correct me if I'm wrong 👍

@Kasher this C++ API needs some serious renovation and this is a separate question, in my opinion.

I suggested to do it in JS, because this bug will still manifest itself if anyone would try to emit('connection', req.connection). Fixing it in JS will make your test case work as well as many other possible variations.

@indutny - Cool. I got it.
Just one last question before I'm implementing this, in order to make sure I got it correctly.
I saw in node_http_parser.cc::Consume that in addition to calling "Consume" on the stream, some callbacks are being set (allocation callback and read callback). Those callbacks are then called with a context, which is the instance of the parser object.
If we remove the call to Consume in _http_server.js::connectionListener, those callbacks won't ever be set with the right context. If, however, we call to Consume, the process will crash because of the assertion in StreamBase.

How should I implement your suggestion? Should I keep the same parser for the socket in _http_server.js, and never free\unconsume it (which means some changes in the function onParserExecuteCommon in _http_server.js)?
Or, should I just skip the call to Consume in _http_server.js::connectionListener if the stream was consumed already, and as a result the allocation callback and the read callback will be called with the wrong context (an instance of a different parser)?
Or, should we add a third c++ method that only sets that callbacks and doesn't call to StreamBase::Consume, and we'll call this method in _http_server.js::connectionListener if we detect that the stream was consumed?

Thanks a lot!!

@Kasher there is a fallback mode there when it checks for _externalStream:

node/lib/_http_server.js

Lines 331 to 335 in a67a04d

    
           var external = socket._handle._externalStream; 
        
           if (external) { 
        
             parser._consumed = true; 
        
             parser.consume(external); 
        
           }

. It would be best to check if the socket was ever consumed and never enter that clause if it was.

This won't affect context of callbacks as the attached http parser (if any) will still be alive until unconsume is called.

@indutny Thanks again for your help.
Maybe I'm missing something, but I'm having trouble understanding how will your suggestion work.
As I wrote before, and as you can see from the following image:

My 'connect' handler is being called from the function onParserExecuteCommon in _http_server.js :

node/lib/_http_server.js

Line 466 in a67a04d

server.emit(eventName, req, socket, bodyHead);

A few lines before my handler is called, there is a call to unconsume on the attached http-parser:

node/lib/_http_server.js

Line 453 in a67a04d

unconsume(parser, socket);

Hence, when my connect handler is called, there is no http-parser attached to the socket. If I remove the call to consume as you suggested, no parser will be attached at all, and no callbacks will be set to the socket's _externalStream.
Do you suggest to remove the call to unconsume as well, so the parser will still be attached? This will result in a refactor in the onParserExecuteCommon method, since I guess we shouldn't execute the following lines as well:

node/lib/_http_server.js

Lines 450 to 456 in a67a04d

    
           socket.removeListener('data', state.onData); 
        
           socket.removeListener('end', state.onEnd); 
        
           socket.removeListener('close', state.onClose); 
        
           unconsume(parser, socket); 
        
           parser.finish(); 
        
           freeParser(parser, req, null); 
        
           parser = null;

Did I miss something?

Thanks a lot, your help is really appreciated!!!

@kosher consume() is just one way to attach socket, the fallback is to attach socketOnData event listener. I suggest that consume() should not be called if the socket was ever previously consumed (even if it is not consumed right now).

@indutny Thanks again.
I've updated my commit, and I would really appreciate any other comments 👍

Few style nits, otherwise looks good!

Minor style nit:

var external = socket._handle._externalStream;
if (!socket._handle._consumed && external) {
  ...
}

Less indent - awesome 😉

When emitting a 'connection' event on a httpServer, the function connectionListener is called. Then, a new parser is created, and 'consume' method is called on the socket's externalStream. However, if this stream was already consumed and unconsumed, the process crashes with a cpp assert from the 'Consume' method in stream_base.h. This commit makes sure that no SIGABRT will be raised and the process will stay alive (after emitting the socket).

Looks like it's ready for a CI: https://ci.nodejs.org/job/node-test-pull-request/6103/

@indutny @joyeecheung

The linux tester failed for fedora24 - 32bit on the test sequential/test-child-process-pass-fd
Do you think it is related to my changes?
I've tried to run this test like 10 times, and it always passes (I'm testing it on ubuntu 14.04).
This test did pass for fedora24 - 64bit though.

So, just to verify - is this by any chance a known issue? If it isn't - I'll download the appropriate iso and create a VM to test this.

Thanks.

Yeah it's a known issue(#11041), also the arm failure is actually not a failure, it's related to some infra issue so it's actually green in the webpage. I'll say CI is green for this PR.

@joyeecheung Awesome, thanks!

When emitting a 'connection' event on a httpServer, the function connectionListener is called. Then, a new parser is created, and 'consume' method is called on the socket's externalStream. However, if this stream was already consumed and unconsumed, the process crashes with a cpp assert from the 'Consume' method in stream_base.h. This commit makes sure that no SIGABRT will be raised and the process will stay alive (after emitting the socket). PR-URL: #11015 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 3e3bfc5

When emitting a 'connection' event on a httpServer, the function connectionListener is called. Then, a new parser is created, and 'consume' method is called on the socket's externalStream. However, if this stream was already consumed and unconsumed, the process crashes with a cpp assert from the 'Consume' method in stream_base.h. This commit makes sure that no SIGABRT will be raised and the process will stay alive (after emitting the socket). PR-URL: #11015 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

When emitting a 'connection' event on a httpServer, the function connectionListener is called. Then, a new parser is created, and 'consume' method is called on the socket's externalStream. However, if this stream was already consumed and unconsumed, the process crashes with a cpp assert from the 'Consume' method in stream_base.h. This commit makes sure that no SIGABRT will be raised and the process will stay alive (after emitting the socket). PR-URL: nodejs#11015 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

When emitting a 'connection' event on a httpServer, the function connectionListener is called. Then, a new parser is created, and 'consume' method is called on the socket's externalStream. However, if this stream was already consumed and unconsumed, the process crashes with a cpp assert from the 'Consume' method in stream_base.h. This commit makes sure that no SIGABRT will be raised and the process will stay alive (after emitting the socket). PR-URL: #11015 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Node 7 does not include nodejs/node#11015. Without this fix, the process is killed when proxying a connection through HTTPS. We could try to work around this on our end, but node 7 is officially unsupported, and it seems more sensible to simply drop support here entirely too.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. lts-watch-v6.x labels Jan 26, 2017

Kasher mentioned this pull request Jan 26, 2017

Assert is raised when emitting 'connection' event #11017

Closed

Kasher changed the title src: unconsme stream fix src: unconsume stream fix Jan 26, 2017

Kasher force-pushed the stream_base_unconsume branch from 91c2f70 to bc8ea9d Compare January 26, 2017 13:04

nodejs-github-bot added dont-land-on-v7.x and removed lts-watch-v6.x labels Jan 26, 2017

Fishrock123 requested a review from indutny January 26, 2017 16:07

mscdex added the http Issues or PRs related to the http subsystem. label Jan 26, 2017

Kasher force-pushed the stream_base_unconsume branch from bc8ea9d to e3fec99 Compare January 27, 2017 13:24

nodejs-github-bot added lts-watch-v6.x and removed dont-land-on-v7.x labels Jan 27, 2017

Kasher force-pushed the stream_base_unconsume branch from e3fec99 to cd3f670 Compare January 27, 2017 20:59

nodejs-github-bot removed lts-watch-v6.x labels Jan 27, 2017

indutny suggested changes Jan 28, 2017

View reviewed changes

Kasher force-pushed the stream_base_unconsume branch from cd3f670 to 1701267 Compare January 28, 2017 10:37

Kasher force-pushed the stream_base_unconsume branch from bd2e905 to 00a6340 Compare January 29, 2017 20:39

nodejs-github-bot added lts-watch-v6.x labels Jan 29, 2017

jasnell approved these changes Feb 2, 2017

View reviewed changes

jasnell closed this Feb 2, 2017

italoacasas mentioned this pull request Feb 5, 2017

Proposal - 7.6.0 (Current) #11185

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

joyeecheung mentioned this pull request Mar 28, 2017

Suspect memory leak in v6.10.1 #12033

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 28, 2017

gibfahn mentioned this pull request Mar 31, 2017

Regressions in v4.8.1 and v6.10.1 nodejs/Release#193

Closed

src: unconsume stream fix by Kasher · Pull Request #11015 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: unconsume stream fix - #11015

src: unconsume stream fix#11015Kasher wants to merge 1 commit intonodejs:masternodejs/node:masterfrom Kasher:stream_base_unconsumeKasher/node:stream_base_unconsumeCopy head branch name to clipboard

Conversation

Kasher commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 commented Jan 26, 2017

Uh oh!

indutny commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kasher commented Jan 26, 2017

Uh oh!

indutny commented Jan 26, 2017

Uh oh!

Kasher commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented Jan 26, 2017

Uh oh!

Kasher commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented Jan 27, 2017

Uh oh!

Kasher commented Jan 27, 2017

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

indutny Jan 28, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 30, 2017

Uh oh!

Kasher commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Jan 31, 2017

Uh oh!

Kasher commented Jan 31, 2017

Uh oh!

jasnell commented Feb 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: unconsume stream fix#11015
Kasher wants to merge 1 commit into
nodejs:masterfrom
Kasher:stream_base_unconsume

Kasher commented Jan 26, 2017 •
edited
Loading

indutny commented Jan 26, 2017 •
edited
Loading

Kasher commented Jan 26, 2017 •
edited
Loading

Kasher commented Jan 27, 2017 •
edited
Loading

Kasher commented Jan 31, 2017 •
edited
Loading