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.

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

http2: fix condition where data is lost #18895

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

Fix a condition that would make a Http2Stream lose some data if pipe is used with a very short stream.

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)

http2

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Feb 21, 2018
stream.push(null);
stream[kMaybeDestroy](null, code);

// defer this until we actuallhy emit end
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

stream.push(null);
stream[kMaybeDestroy]();

// defer this until we actuallhy emit end
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@mcollina
Copy link
Member Author

stream.on('end', stream[kMaybeDestroy]);
stream.push(null);

// TODO mcollina: are we sure this is correct?
Copy link
Member

Choose a reason for hiding this comment

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

The comment style we generally use for these is // TODO(mcollina): …

// TODO mcollina: are we sure this is correct?
if (stream.readableLength === 0) {
// autoresume, the stream needs to finish
stream.resume()
Copy link
Member

Choose a reason for hiding this comment

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

What net.Sockets do is calling .read(0) unconditionally after .push(null). Could you try to see if that makes sense? I would really like to merge these bits of code and it would be great if we could avoid these discrepancies…

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I think it would be nice to combine the two tests as they are so similar, most parts could be reused.

this.closed) {
this.destroy();
// this should return, but eslint complains
// return
Copy link
Member

Choose a reason for hiding this comment

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

The last return is just considered obsolete. I am not sure if the comment really adds a benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

the comment states that, if any more code is added in there, a return should be included.
This makes it explicit. Can you recommend some alternative wording that are ok for you?

(I'm generally 👎 on that eslint rule)

stream.push(null);
stream[kMaybeDestroy](null, code);

// defer this until we actually emit end
Copy link
Member

Choose a reason for hiding this comment

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

Would you be so kind and upper case the first character of each comment and punctuate the comments? :-)

stream[kMaybeDestroy]();
} else {
stream.on('end', stream[kMaybeDestroy]);
stream.push(null);
Copy link
Member

Choose a reason for hiding this comment

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

Is the stream.push necessary here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it is. It's needed to close the stream.

!this.closed)) {
return;
}
if (error || code !== NGHTTP2_NO_ERROR) {
Copy link
Member

Choose a reason for hiding this comment

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

I would move the code !== NGHTTP2... check to the next if. That way it is clearer that destroy is not called with error in that case. Or do I miss something here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The code is correct as it is. There can be an error (from net or tls) or the stream can return an error code - but that's not mapped to a JS ERROR yet.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is if code !== NGHTTP2_NO_ERROR, error will be falsy. And in that case error does not have to be passed to this.destroy. This is the case in the next if statement and therefore I would move the code !== NGHTTP2... case down.

But it is not important.

@BridgeAR
Copy link
Member

Could this maybe also have some influence on some of our test failures?

@mcollina
Copy link
Member Author

@BridgeAR yes, this could be the cause of some of our spurious failures.

@mcollina
Copy link
Member Author

@addaleax PTAL

Can you also check if that TODO that I left is correct? I'm starting to think it is.

return;
}

// TODO mcollina: remove usage of _*State properties
Copy link
Member

Choose a reason for hiding this comment

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

This TODO is still in the previous style ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@mcollina
Copy link
Member Author

I think it would be nice to combine the two tests as they are so similar, most parts could be reused.

The two tests checks two different things, one for the compat API and one for the stream. I would keep them separate as it might be more maintainable in the long run. We have a really poor isolation logic between various sub-tests.

@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

Landed in dbe645f

@mcollina mcollina closed this Feb 26, 2018
@mcollina mcollina deleted the fix-http2-destroy-flow branch February 26, 2018 17:37
mcollina added a commit that referenced this pull request Feb 26, 2018
PR-URL: #18895
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018
PR-URL: nodejs#18895
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
PR-URL: nodejs#18895
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #20456
PR-URL: #18895
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18895
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Backport-PR-URL: #20456
PR-URL: #18895
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Backport-PR-URL: #20456
PR-URL: #18895
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants