GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Revert "src: let http2 streams end after session close" by santigimeno · Pull Request #46249 · nodejs/node

Conversation

@santigimeno

@santigimeno santigimeno commented Jan 18, 2023

Copy link
Copy Markdown
Member

This reverts commit dee882e. Moved the test that demonstrated what this commit was fixing to the known_issues folder.

Fixes: #46234

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@santigimeno
Revert "src: let http2 streams end after session close"
3619cb3 
This reverts commit dee882e.
Moved the test that demonstrated what this commit was fixing to the
`known_issues` folder.

Fixes: nodejs#46234
@nodejs-github-bot

nodejs-github-bot commented Jan 18, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jan 18, 2023
@santigimeno santigimeno mentioned this pull request Jan 18, 2023
Node 18.13 HTTP/2 live lock #46234
Closed
@santigimeno

santigimeno commented Jan 18, 2023

Copy link
Copy Markdown
Member Author

Once this is merged, should we reopen #42713?
lpinca reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag approved these changes Jan 18, 2023
View reviewed changes
@ronag

ronag commented Jan 18, 2023

Copy link
Copy Markdown
Member

Would be nice with a regression test if possible.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@santigimeno

santigimeno commented Jan 18, 2023

Copy link
Copy Markdown
Member Author

Would be nice with a regression test if possible.

I moved the original test to the known_issues folder. Is that what you're asking?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Jan 18, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina

mcollina commented Jan 18, 2023

Copy link
Copy Markdown
Member

@nodejs/releasers this should be backported to v19.x, v18.x, and v16.x.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina added v16.x labels Jan 18, 2023
RafaelGSS
RafaelGSS approved these changes Jan 18, 2023
View reviewed changes
cjihrig
cjihrig approved these changes Jan 18, 2023
View reviewed changes
lpinca
lpinca approved these changes Jan 18, 2023
View reviewed changes
@RafaelGSS RafaelGSS added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Jan 19, 2023
@github-actions

github-actions Bot commented Jan 19, 2023

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @RafaelGSS. Please 👍 to approve.
RafaelGSS, ShogunPanda, ronag, and anonrig reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS

RafaelGSS commented Jan 19, 2023

Copy link
Copy Markdown
Member

I requested the fast-track to include it in the next v19.x release (planning to create the proposal between Thursday and Friday).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 20, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 20, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 20, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49097/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 20, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49108/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Jan 20, 2023

Copy link
Copy Markdown
Member

The tests keep failing.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Jan 21, 2023
CI Reliability 2023-01-21 nodejs/reliability#490
Open
CI Reliability 2023-01-22 nodejs/reliability#491
Open
CI Reliability 2023-01-23 nodejs/reliability#492
Open
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 8, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 8, 2023
@github-actions

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown
Contributor
Failed to start CI
- Validating Jenkins credentials
✖  Jenkins credentials invalid
https://github.com/nodejs/node/actions/runs/4126652112
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. label Feb 8, 2023
@mcollina

mcollina commented Feb 9, 2023

Copy link
Copy Markdown
Member

yes, this can land.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. labels Feb 17, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 17, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 17, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49608/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Feb 18, 2023
CI Reliability 2023-02-18 nodejs/reliability#499
Open
32 tasks
@nodejs-github-bot

nodejs-github-bot commented Feb 18, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49690/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Feb 18, 2023

Copy link
Copy Markdown
Member

@santigimeno Can you run make lint-js-fix to add the comma in the test now required by linting rules and then push up a fixup commit? I tried to do it for you to save you the annoyance, but I don't have permission to force push to your branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott mentioned this pull request Feb 18, 2023
Revert "src: let http2 streams end after session close" #46721
Merged
@Trott

Trott commented Feb 18, 2023

Copy link
Copy Markdown
Member

@santigimeno Can you run make lint-js-fix to add the comma in the test now required by linting rules and then push up a fixup commit? I tried to do it for you to save you the annoyance, but I don't have permission to force push to your branch.

In case @santigimeno isn't around over the weekend or whatever, I've opened #46721 which preserves their authorship of the commit but adds the lint fix. Maybe head over and approve/fast-track that one?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

richardlau commented Feb 18, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Isn't the without ssl failure still unresolved?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Feb 19, 2023
CI Reliability 2023-02-19 nodejs/reliability#500
Open
21 tasks
@Trott

Trott commented Feb 19, 2023

Copy link
Copy Markdown
Member

Isn't the without ssl failure still unresolved?

I think all that needs to happen for that is to change common.skip to assert.fail in the test and leave a comment that when the issue is fixed and it's moved out of known_issues, it needs to be changed back.

I'll leave a suggestion.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott reviewed Feb 19, 2023
View reviewed changes
Comment thread test/known_issues/test-http2-trailers-after-session-close.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Update test/known_issues/test-http2-trailers-after-session-close.js
cceb018
@Trott Trott added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Feb 19, 2023
Trott
Trott reviewed Feb 19, 2023
View reviewed changes
Comment thread test/known_issues/test-http2-trailers-after-session-close.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Update test/known_issues/test-http2-trailers-after-session-close.js
2f640df
Trott
Trott reviewed Feb 19, 2023
View reviewed changes
Comment thread test/known_issues/test-http2-trailers-after-session-close.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Update test/known_issues/test-http2-trailers-after-session-close.js
f3d066c
@github-actions github-actions Bot mentioned this pull request Feb 20, 2023
CI Reliability 2023-02-20 nodejs/reliability#501
Open
24 tasks
@santigimeno

santigimeno commented Feb 22, 2023

Copy link
Copy Markdown
Member Author

This can be closed now as #46721 landed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@santigimeno santigimeno closed this Feb 22, 2023
@BethGriggs BethGriggs removed the lts-watch-v16.x label Mar 27, 2023
@targos targos added backported-to-v18.x and removed lts-watch-v18.x labels Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments
@mcollina mcollina mcollina approved these changes
@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
@ronag ronag ronag approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes
+1 more reviewer
@fhinkel fhinkel fhinkel approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. fast-track PRs proposed for a shorter-than-standard waiting period before landing. http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Node 18.13 HTTP/2 live lock

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@santigimeno @nodejs-github-bot @ronag @mcollina @RafaelGSS @Trott @richardlau @fhinkel @lpinca @cjihrig @targos @BethGriggs
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.