test: http2 rstStream duplicate call by trivikr · Pull Request #16217 · nodejs/node

trivikr · 2017-10-15T09:50:24Z

This commit fixes an issue with rstStream() which checked incorrect value of rst. It also adds a test case for this fix

Lines 928 to 933 in 411695e

    
           if (this[kState].rst) { 
        
             // rst has already been called, do not call again, 
        
             // skip straight to destroy 
        
             stream.destroy(); 
        
             return; 
        
           }

Refs: #14985

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)

test, http2

apapirovski

Looks great! Just a minor nit.

apapirovski · 2017-10-15T15:47:42Z

If you call the second client.rstStream with a different code, you could then verify that the rstCode didn't get set to the 2nd incorrect code (the strictEqual check below will do the job). Right now it's a bit ambiguous as to whether it does or not.

I've updated the second call to send different code (number 1) which is not updated in req.rstCode

apapirovski · 2017-10-15T15:47:50Z

Nice catch!

Thanks! :-)

This commit fixes an issue with rstStream() which checked incorrect value of rst. It also adds a test case for this fix Refs: nodejs#14985

mcollina · 2017-10-17T08:01:43Z


  const req = client.request({ ':path': '/' });
+  // make sure that destroy is called twice
+  req._destroy = common.mustCall(req._destroy.bind(req), 2);


I am a bit lost in how this is possible, in theory it is protected from running _destroy twice: https://github.com/nodejs/node/blob/master/lib/internal/streams/destroy.js#L10-L30

It was confusing for me too.
The two instances where stream.destroy() is called are:

in first call of rstStream()

node/lib/internal/http2/core.js

Line 631 in ff747e3

stream.destroy();

in second call of rstStream()

node/lib/internal/http2/core.js

Line 931 in ff747e3

stream.destroy();

This might translate to two calls of req._destroy?

I've run the test on repeat 100 times to confirm that it it succeeds using the following command:

> python tools/test.py -J --mode=release parallel/test-http2-client-rststream-before-connect --repeat 100 [00:06|% 100|+ 100|- 0]: Done

I believe @mcollina is right. I think what's happening is that this line is accounting for the 2 triggers of _destroy:

node/lib/internal/http2/core.js

Lines 1494 to 1498 in ff747e3

if (this[kID] === undefined) {

debug(`[${sessionName(session[kType])}] queuing destroy for new stream`);

this.once('ready', this._destroy.bind(this, err, callback));

return;

}

The reason that it's different than before is because the declaration was moved before client.rstStream() instead of after. I think that stream.destroy() isn't even necessary and it should just be the empty return;.

ping @jasnell — any thoughts re: whether that stream.destroy() serves an actual purpose?

I've run the test on repeat 100 times

@trivikr the test might be passing, but I'm wondering if the actual behavior is correct.

I don't think that calling rstStream() twice should destroy the stream. It seems a not needed side effect. I think it should throw.

node/lib/internal/http2/core.js

Lines 929 to 931 in ff747e3

// rst has already been called, do not call again,

// skip straight to destroy

stream.destroy();

cc @jasnell

@mcollina I ran the test on repeat 100 times to check if there's any flakiness in it.
I'll wait for the response from @jasnell and update the code and test as per the recommendation at the end of the discussion.

I'll dig in on this a bit more later on today

BridgeAR · 2017-10-18T15:01:09Z

I would argue that this actually is a http2 fix and not a test change. So the http2 subsystem should be used instead.

apapirovski

Just making it explicit this shouldn't land until the _destroy situation is addressed, one way or another.

trivikr · 2017-10-25T18:24:52Z

Any update on this request?

apapirovski · 2017-11-07T17:22:32Z

Fixed in #16753

nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Oct 15, 2017

apapirovski approved these changes Oct 15, 2017

View reviewed changes

test: http2 rstStream duplicate call

810edb8

This commit fixes an issue with rstStream() which checked incorrect value of rst. It also adds a test case for this fix Refs: nodejs#14985

trivikr force-pushed the test-http2-client-rststream branch from 7e7f4d6 to 810edb8 Compare October 15, 2017 16:05

jasnell approved these changes Oct 15, 2017

View reviewed changes

lpinca approved these changes Oct 16, 2017

View reviewed changes

mcollina reviewed Oct 17, 2017

View reviewed changes

apapirovski suggested changes Oct 19, 2017

View reviewed changes

apapirovski closed this Nov 7, 2017

trivikr deleted the test-http2-client-rststream branch November 7, 2017 18:54

	if (this[kState].rst) {
	// rst has already been called, do not call again,
	// skip straight to destroy
	stream.destroy();
	return;
	}

	if (this[kID] === undefined) {
	debug(`[${sessionName(session[kType])}] queuing destroy for new stream`);
	this.once('ready', this._destroy.bind(this, err, callback));
	return;
	}

Uh oh!

Conversation

trivikr commented Oct 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trivikr Oct 15, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 15, 2017

Choose a reason for hiding this comment

Uh oh!

trivikr Oct 15, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

trivikr Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina Oct 18, 2017

Choose a reason for hiding this comment

Uh oh!

trivikr Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Oct 18, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Oct 18, 2017

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

trivikr commented Oct 25, 2017

Uh oh!

apapirovski commented Nov 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

apapirovski Oct 15, 2017 •

edited

Loading

trivikr Oct 17, 2017 •

edited

Loading

trivikr Oct 18, 2017 •

edited

Loading