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.

Sign up for GitHub

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

Jump to bottom

test: http2 rstStream duplicate call #16217

Closed
wants to merge 1 commit into from
Closed

test: http2 rstStream duplicate call #16217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,7 @@ class Http2Session extends EventEmitter {
'number');
}

if (this[kState].rst) {
if (stream[kState].rst) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :-)

// rst has already been called, do not call again,
// skip straight to destroy
stream.destroy();
Expand Down
9 changes: 6 additions & 3 deletions test/parallel/test-http2-client-rststream-before-connect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@ server.on('listening', common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@trivikr trivikr Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@trivikr trivikr Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


client.rstStream(req, 0);
// redundant call to rstStream with new code
client.rstStream(req, 1);
// confirm that rstCode is from the first call to rstStream
assert.strictEqual(req.rstCode, 0);

// make sure that destroy is called
req._destroy = common.mustCall(req._destroy.bind(req));

req.on('streamClosed', common.mustCall((code) => {
assert.strictEqual(req.destroyed, true);
assert.strictEqual(code, 0);
Expand Down