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: make compat finished match http/1 #24347

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Nov 13, 2018

Looking at the http1 semantic the current http2 compat implementation of finished seems rather complicated and fragile? Also it doesn't match the semantics of http1.

I have one http2 compat test failing which I don't quite undertand the purpose of. Am I missing something critical here?

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Refs: #24743

NOTE TO SELF: review cb behaviour when ending and destroyed. Is ´onStreamCloseResponse` required?

@nodejs-github-bot nodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. labels Nov 13, 2018
@ronag ronag force-pushed the http2-simplify-finished branch 4 times, most recently from e651c5b to 58be934 Compare November 14, 2018 12:18
@ronag
Copy link
Member Author

ronag commented Nov 14, 2018

The interesting part here is that in http1 finished doesn't seem to care whether the socket is closed or not, it only cares about whether end has been called or not. While in http2-compat it doesn't really care that much about end, instead if mostly cares about whether the socket is closed or not.

I think this discrepancy is also missing a test somewhere? @mcollina

EDIT: See, fab97ee

@ronag ronag changed the title Http2 simplify finished http2: compat simplify finished Nov 14, 2018
lib/internal/http2/compat.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the http2-simplify-finished branch 4 times, most recently from ae5a5f0 to e164f4e Compare November 14, 2018 12:39
lib/internal/http2/compat.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Member

@jasnell can you help here?

@mcollina
Copy link
Member

The interesting part here is that in http1 finished doesn't seem to care whether the socket is closed or not, it only cares about whether end has been called or not. While in http2-compat it doesn't really care that much about end, instead if mostly cares about whether the socket is closed or not.

I think this discrepancy is also missing a test somewhere? @mcollina

I think this is just a difference that we need to fix.

@ronag
Copy link
Member Author

ronag commented Nov 14, 2018

I'm actually a bit unsure what happens if you call end on a closed connection in http1?

@ronag ronag force-pushed the http2-simplify-finished branch 2 times, most recently from 7a57643 to d9312e2 Compare November 14, 2018 19:33
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test that shows what you are trying to achieve?

lib/internal/http2/compat.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Member

I'm actually a bit unsure what happens if you call end on a closed connection in http1?

if the stream is finished, nothing happens on multiple end() in HTTP1

node/lib/_http_outgoing.js

Lines 683 to 684 in b32c5f0

if (this.finished) {
return this;
.

If you are still unsure, have you trying preparing an example?

@ronag
Copy link
Member Author

ronag commented Nov 20, 2018

@mcollina

if the stream is finished, nothing happens on multiple end() in HTTP1

This is if you have called end() previously. But what if you call end for the first time after the socket has aborted.

@mcollina
Copy link
Member

Can you create an example to check this?

@jasnell
Copy link
Member

jasnell commented Nov 21, 2018

Sorry @mcollina, I just spotted the notification on this. I'll queue this one up and take a look hopefully later today.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

I'm sorry, but this is completely incorrect. There's a good reason that this makes a test fail.

@ronag
Copy link
Member Author

ronag commented Nov 29, 2018

@apapirovski care to expand on that?

The failing test looks wrong for me:

const server = createServer(mustCall((request, response) => {
    strictEqual(response.finished, true);
    response.writeHead(HTTP_STATUS_OK, { foo: 'bar' });
    response.end('data', mustCall());
  }));

It test that respone.finished is true before end() is called which is not how http/1 behaves.

@ronag ronag force-pushed the http2-simplify-finished branch 3 times, most recently from a518fe2 to 252a03f Compare November 29, 2018 21:47
@ronag
Copy link
Member Author

ronag commented Jan 11, 2020

@Trott: CI failed due to merge conflict. Please try again.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM as a bug fix

@nodejs-github-bot
Copy link
Collaborator

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2020
@nodejs-github-bot
Copy link
Collaborator

@ronag

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Feb 17, 2020

@nodejs/build I seem to be having problems with node-test-binary-arm-12+ in multiple PR's. Even those that just touch docs? #31805

@rvagg
Copy link
Member

rvagg commented Feb 17, 2020

Thanks @ronag, don't treat this as a blocker for your code, it's a Jenkins problem on our end.

@nodejs/build we're having a commit-ref propagation problem, in https://ci.nodejs.org/job/node-test-pull-request/29176/:

anyone got time to look at this one? @joaocgreis I bet you'd have the most insight and could see the problem quickest if you're available.

@ronag
Copy link
Member Author

ronag commented Feb 17, 2020

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 18, 2020

@joaocgreis
Copy link
Member

Please do not use the "Resume build" when the PR changes. The "Resume build" only starts jobs that failed, so jobs that passed before will remain green, even if they would fail with the new changes. The "Resume build" feature is a source of problems, but it's the only way we have to reduce the impact of infra failures.

Please use "Rebuild" instead when there are any changes in the PR.

Both e6a54b6 and db24c25 were the head of #31805 at some point. Only the node-test-binary-arm-12+ job was started in https://ci.nodejs.org/job/node-test-pull-request/29176/. The other two were re-used from a previous PR because they passed, so the compile job never ran for the latest head.

ronag added a commit that referenced this pull request Feb 18, 2020
finished should true directly after end().

PR-URL: #24347
Refs: #24743
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ronag
Copy link
Member Author

ronag commented Feb 18, 2020

Landed in 8ba7a2f

@ronag ronag closed this Feb 18, 2020
codebytere pushed a commit that referenced this pull request Feb 27, 2020
finished should true directly after end().

PR-URL: #24347
Refs: #24743
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Feb 29, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
finished should true directly after end().

PR-URL: #24347
Refs: #24743
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
finished should true directly after end().

PR-URL: #24347
Refs: #24743
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
finished should true directly after end().

PR-URL: #24347
Refs: #24743
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.