src: unconsume stream fix #11015

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
@Kasher
Contributor

Kasher commented Jan 26, 2017

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

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

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Jan 26, 2017

Member

R= @indutny probably?

Member

Fishrock123 commented Jan 26, 2017

R= @indutny probably?

@Fishrock123 Fishrock123 requested a review from indutny Jan 26, 2017

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Jan 26, 2017

Member

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?

Member

indutny commented Jan 26, 2017

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?

@Kasher

This comment has been minimized.

Show comment
Hide comment
@Kasher

Kasher Jan 26, 2017

Contributor

@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 👍

Contributor

Kasher commented Jan 26, 2017

@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 👍

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Jan 26, 2017

Member

@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.

Member

indutny commented Jan 26, 2017

@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.

@Kasher

This comment has been minimized.

Show comment
Hide comment
@Kasher

Kasher Jan 26, 2017

Contributor

@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!!

Contributor

Kasher commented Jan 26, 2017

@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!!

@mscdex mscdex added the http label Jan 26, 2017

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Jan 26, 2017

Member

@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.

Member

indutny commented Jan 26, 2017

@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.

@Kasher

This comment has been minimized.

Show comment
Hide comment
@Kasher

Kasher Jan 27, 2017

Contributor

@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 :

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:

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!!!

Contributor

Kasher commented Jan 27, 2017

@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 :

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:

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!!!

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Jan 27, 2017

Member

@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).

Member

indutny commented Jan 27, 2017

@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).

@Kasher

This comment has been minimized.

Show comment
Hide comment
@Kasher

Kasher Jan 27, 2017

Contributor

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

Contributor

Kasher commented Jan 27, 2017

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

@indutny

Few style nits, otherwise looks good!

lib/_http_server.js
- parser._consumed = true;
- parser.consume(external);
+ // We only consume the socket if it has never been consumed before.
+ if (!socket._handle._consumed) {

This comment has been minimized.

@indutny

indutny Jan 28, 2017

Member

Minor style nit:

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

Less indent - awesome 😉

@indutny

indutny Jan 28, 2017

Member

Minor style nit:

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

Less indent - awesome 😉

@Kasher

This comment has been minimized.

Show comment
Hide comment
@Kasher

Kasher Jan 28, 2017

Contributor

@indutny I've made the requested changes 👍

Contributor

Kasher commented Jan 28, 2017

@indutny I've made the requested changes 👍

@indutny

Thank you! Few more changes before landing. Looks really good now!

test/sequential/test-http-server-unconsume-consume.js
+
+
+function child() {
+ const testServer = http.createServer(function(req, res) {

This comment has been minimized.

@indutny

indutny Jan 28, 2017

Member

Needs common.mustCall wrapper, also could use arrow syntax: (req, res) => {

@indutny

indutny Jan 28, 2017

Member

Needs common.mustCall wrapper, also could use arrow syntax: (req, res) => {

test/sequential/test-http-server-unconsume-consume.js
+ res.writeHead(200, {});
+ res.end();
+ });
+ testServer.on('connect', function(req, socket, head) {

This comment has been minimized.

@indutny

indutny Jan 28, 2017

Member

common.mustCall and arrow syntax here too

@indutny

indutny Jan 28, 2017

Member

common.mustCall and arrow syntax here too

test/sequential/test-http-server-unconsume-consume.js
+ testServer.emit('connection', socket);
+ testServer.close();
+ });
+ testServer.listen(0, function() {

This comment has been minimized.

@indutny

indutny Jan 28, 2017

Member

Arrows! :)

@indutny

indutny Jan 28, 2017

Member

Arrows! :)

test/sequential/test-http-server-unconsume-consume.js
+else
+ parent();
+
+function parent() {

This comment has been minimized.

@indutny

indutny Jan 28, 2017

Member

Why does it have to be this way? It is ok if the test crashes or exits with non-zero code. No need to wrap it up this way.

@indutny

indutny Jan 28, 2017

Member

Why does it have to be this way? It is ok if the test crashes or exits with non-zero code. No need to wrap it up this way.

@Kasher

This comment has been minimized.

Show comment
Hide comment
@Kasher

Kasher Jan 29, 2017

Contributor

@indutny, Thanks again!!!
Made the changes.
Regarding line 5 in the test (const testServer = http.createServer((req, res) => { ) - This won't be called at all, I just added kind of an empty callback, since without it the code looks a bit odd in my opinion...
That's why this function isn't wrapped by common.mustCall. If you prefer, I can remove this function entirely.

Thanks!

Contributor

Kasher commented Jan 29, 2017

@indutny, Thanks again!!!
Made the changes.
Regarding line 5 in the test (const testServer = http.createServer((req, res) => { ) - This won't be called at all, I just added kind of an empty callback, since without it the code looks a bit odd in my opinion...
That's why this function isn't wrapped by common.mustCall. If you prefer, I can remove this function entirely.

Thanks!

@indutny

Very last nit, but in general already LGTM. Thank you!

test/parallel/test-http-server-unconsume-consume.js
+const http = require('http');
+
+const testServer = http.createServer((req, res) => {
+ res.end();

This comment has been minimized.

@indutny

indutny Jan 29, 2017

Member

Let's add common.fail('Should not be called') then.

@indutny

indutny Jan 29, 2017

Member

Let's add common.fail('Should not be called') then.

This comment has been minimized.

@Kasher

Kasher Jan 29, 2017

Contributor

👍

@Kasher

Kasher Jan 29, 2017

Contributor

👍

src: unconsume stream fix
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).
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
Member

joyeecheung commented Jan 30, 2017

@Kasher

This comment has been minimized.

Show comment
Hide comment
@Kasher

Kasher Jan 31, 2017

Contributor

@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.

Contributor

Kasher commented Jan 31, 2017

@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.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Jan 31, 2017

Member

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.

Member

joyeecheung commented Jan 31, 2017

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.

@Kasher

This comment has been minimized.

Show comment
Hide comment
@Kasher

Kasher Jan 31, 2017

Contributor

@joyeecheung Awesome, thanks!

Contributor

Kasher commented Jan 31, 2017

@joyeecheung Awesome, thanks!

@jasnell

jasnell approved these changes Feb 2, 2017

jasnell added a commit that referenced this pull request Feb 2, 2017

src: unconsume stream fix in internal http impl
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>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 2, 2017

Member

Landed in 3e3bfc5

Member

jasnell commented Feb 2, 2017

Landed in 3e3bfc5

@jasnell jasnell closed this Feb 2, 2017

italoacasas added a commit that referenced this pull request Feb 3, 2017

src: unconsume stream fix in internal http impl
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>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

src: unconsume stream fix in internal http impl
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>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

src: unconsume stream fix in internal http impl
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>

jasnell added a commit that referenced this pull request Mar 7, 2017

src: unconsume stream fix in internal http impl
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>

jasnell added a commit that referenced this pull request Mar 7, 2017

src: unconsume stream fix in internal http impl
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>

MylesBorins added a commit that referenced this pull request Mar 9, 2017

src: unconsume stream fix in internal http impl
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>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

MylesBorins added a commit that referenced this pull request Mar 9, 2017

src: unconsume stream fix in internal http impl
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>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v4.8.1 proposal #11760

@gibfahn gibfahn referenced this pull request in nodejs/Release Mar 31, 2017

Closed

Regressions in v4.8.1 and v6.10.1 #193

pimterry added a commit to pimterry/mockttp that referenced this pull request Nov 30, 2017

Stop testing on node 7 due to unfixed bugs
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment