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: refactor error handling" #15047

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 26, 2017

This reverts commit 4ca8ff2.

After this lands (hopefully promptly), #14991 should be re-opened and re-landed after the test failures are fixed.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
Affected core subsystem(s)

http2

@nodejs-github-bot nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Aug 26, 2017
@Trott
Copy link
Member Author

Trott commented Aug 26, 2017

@jasnell @mcollina

@Trott
Copy link
Member Author

Trott commented Aug 26, 2017

Sample test failure this revert will fix:

https://ci.nodejs.org/job/node-test-binary-windows/10729/RUN_SUBSET=1,VS_VERSION=vcbt2015,label=win10/console

not ok 202 parallel/test-http2-respond-file-404
  ---
  duration_ms: 0.170
  severity: fail
  stack: |-
    (node:4708) ExperimentalWarning: The http2 module is an experimental API.
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'ENOENT: no such file or directory, open \'c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\1\\VS_VERSION\\vcbt2015\\label\\ === 'ENOENT: no such file or directory, open \'./not-a-file\''
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\common\index.js:715:16
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\common\index.js:509:15
        at onError (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\parallel\test-http2-respond-file-404.js:25:9)
        at ServerHttp2Stream.afterOpen (internal/http2/core.js:1704:7)
        at FSReqWrap.oncomplete (fs.js:135:15)

@jasnell
Copy link
Member

jasnell commented Aug 26, 2017

O.o not sure how that happened but lgtm! And +1 to fast track

@Trott
Copy link
Member Author

Trott commented Aug 26, 2017

@jasnell
Copy link
Member

jasnell commented Aug 26, 2017

Have a distinct feeling I simply missed the failure before landing due to the issues we were having with ci. Good catch on it. I'll reopen the pr and revisit on Monday

@jasnell
Copy link
Member

jasnell commented Aug 26, 2017

Thank you @Trott!

@Trott
Copy link
Member Author

Trott commented Aug 26, 2017

due to the issues we were having with ci

Yeah, no kidding. As if we needed it, it's a good reminder of the importance of keeping CI reliably green. Otherwise, it gets ignored when the red actually means something.

On that front, I went in and fixed the reliably-failing CentOS 5 test machine. (I usually don't bother because I feel like I don't know what I'm doing. But it's a weekend and how much damage could I do by logging in and restarting the Jenkins agent? Didn't actually think it would work but...it did.)

So, hopefully once this lands, we're back to green, or at least yellow.

This reverts commit 4ca8ff2.

That commit was landed without a green CI and is failing on Windows.
Trott added a commit to Trott/io.js that referenced this pull request Aug 26, 2017
This reverts commit 4ca8ff2.

That commit was landed without a green CI and is failing on Windows.

PR-URL: nodejs#15047
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Aug 26, 2017

Landed in f912080

@Trott Trott closed this Aug 26, 2017
@mcollina
Copy link
Member

Sorry for this, I had a couple of busy days and I didn't check if there were problems in CI. I typically check when landing. I'll update the original PR.

ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
This reverts commit 4ca8ff2.

That commit was landed without a green CI and is failing on Windows.

PR-URL: nodejs/node#15047
Reviewed-By: James M Snell <jasnell@gmail.com>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
This reverts commit 4ca8ff2.

That commit was landed without a green CI and is failing on Windows.

PR-URL: nodejs/node#15047
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Oct 20, 2018
2 tasks
@Trott Trott deleted the revert-4ca8ff2 branch January 13, 2022 22:46
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