FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Revert "http2: fix end without read" by BridgeAR · Pull Request #20832 · nodejs/node · GitHub

/ node Public

Revert "http2: fix end without read" - #20832

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:fix-ci
ConversationCommits (1)ChecksFiles changed
Closed

Revert "http2: fix end without read"#20832
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:fix-ci

Conversation

BridgeAR commented May 19, 2018

Copy link
Copy Markdown
Member

This reverts commit 8d38288.

Refs: #20621

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

BridgeAR requested review from addaleax and apapirovski May 19, 2018 00:44
nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 19, 2018
BridgeAR mentioned this pull request May 19, 2018
Closed
3 tasks
addaleax approved these changes May 19, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM :(

addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label May 19, 2018
BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 19, 2018

BridgeAR commented May 19, 2018

Copy link
Copy Markdown
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/14969/

apapirovski suggested changes May 19, 2018
View reviewed changes

apapirovski left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should not be reverted. There's a bug in http2 that's just being exacerbated here (well, not truly — it's just showing that this bug exists) but this isn't the root cause.

The test case — as modified here — won't even survive on any version but this one. It has 100% failure prior to this commit.

As for the bug, it's as @BridgeAR stated — same root cause as the already opened issue for all the others. I'm working on a fix but it's complicated, might take another day.

apapirovski commented May 19, 2018

Copy link
Copy Markdown
Contributor

Please consider having a look at #20772 and helping with the effort in there. There are serious bugs in http2 that need to be resolved. Everything works as expected on Linux & macOS now but FreeBSD & Windows assistance is desperately needed. Thanks!

mcollina commented May 19, 2018

Copy link
Copy Markdown
Member

@apapirovski our process is to typically keep the CI green. Unless it's a very easy fix, we prefer to revert quickly and send a follow-up PR that re-add the functionality.

mcollina commented May 19, 2018

Copy link
Copy Markdown
Member

cc @nodejs/tsc.

apapirovski commented May 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@mcollina if we really want to keep the CI green — even though there are currently at least two serious bugs in http2 — then we should revert the change to the test only and not the rest of this PR, which has nothing to do with the CI being red.

From my perspective this might at least encourage people to look at the bug. We are too quick to revert or label things as flaky to achieve perceived stability. Instead those things just end up buried and never attended to in the future. Just my opinion, of course...

addaleax commented May 19, 2018

Copy link
Copy Markdown
Member

We are too quick to revert or label things as flaky to achieve perceived stability.

The goal is not perceived stability, it’s other people’s ability to tell whether the failures they are seeing are coming from their own changes or not.

then we should revert the change to the test only and not the rest of this PR

That seems fine to me too.

addaleax commented May 19, 2018

Copy link
Copy Markdown
Member

Oh, but generally: We should always be quick to revert if anything broke. I still think we should just land this asap and re-open a PR with the original changes, minus test changes.

BridgeAR mentioned this pull request May 19, 2018
Closed
4 tasks

BridgeAR commented May 19, 2018

Copy link
Copy Markdown
Member Author

I opened #20835 as alternative. It just skips the flaky tests in case of failures. I think that is currently the best solution until the underlying issue gets fixed.

BridgeAR closed this May 19, 2018
BridgeAR deleted the fix-ci branch January 20, 2020 11:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
+1 more reviewer
apapirovski apapirovski requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL