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

Revert "http2: fix end without read" #20832

Closed
wants to merge 1 commit into from
Closed

Conversation

BridgeAR
Copy link
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

@nodejs-github-bot 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 BridgeAR mentioned this pull request May 19, 2018
3 tasks
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM :(

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label May 19, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 19, 2018
@BridgeAR
Copy link
Member Author

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.

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
Copy link
Member

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
Copy link
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
Copy link
Member

cc @nodejs/tsc.

@apapirovski
Copy link
Member

apapirovski commented May 19, 2018

@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
Copy link
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
Copy link
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
Copy link
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 BridgeAR closed this May 19, 2018
@BridgeAR BridgeAR deleted the fix-ci branch January 20, 2020 11:33
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. 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
Development

Successfully merging this pull request may close these issues.

5 participants