http2: send error text in case of ALPN mismatch by addaleax · Pull Request #18986 · nodejs/node · GitHub

Send a human-readable HTTP/1 response in case of an unexpected
ALPN protocol. This helps with debugging this condition,
since previously the only result of it would be a closed socket.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

@nodejs/http2

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket.

CI: https://ci.nodejs.org/job/node-test-commit/16487/

LGTM

Landed as d1bc6f0.

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket. PR-URL: #18986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This landed with a failing CI: https://ci.nodejs.org/job/node-test-binary-windows/15280/COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=3/

not ok 229 parallel/test-http2-https-fallback

  ---
  duration_ms: 0.445
  severity: fail
  stack: |-
    (node:5952) ExperimentalWarning: The http2 module is an experimental API.
    assert.js:243
        throw err;
        ^
    
    AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
    
      assert.ok(/Unknown ALPN Protocol, expected `h2` to be available/.test(text))
    
        at TLSSocket.tls.setEncoding.on.on.common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-http2-https-fallback.js:143:9)
        at TLSSocket.<anonymous> (c:\workspace\node-test-binary-windows\test\common\index.js:476:15)
        at TLSSocket.emit (events.js:136:15)
        at endReadableNT (_stream_readable.js:1021:12)
        at process._tickCallback (internal/process/next_tick.js:115:19)
  ...

CI (master): https://ci.nodejs.org/job/node-test-pull-request/13454/

@richardlau PR is coming, I do have a fix.

The test was flaky because it relied on a specific order of asynchronous operation that were fired paralellely. This was true on most platform and conditions, but not all the time. See: nodejs#18986

The test was flaky because it relied on a specific order of asynchronous operation that were fired paralellely. This was true on most platform and conditions, but not all the time. See: #18986 PR-URL: #19093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket. PR-URL: nodejs#18986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The test was flaky because it relied on a specific order of asynchronous operation that were fired paralellely. This was true on most platform and conditions, but not all the time. See: nodejs#18986 PR-URL: nodejs#19093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket. PR-URL: nodejs#18986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The test was flaky because it relied on a specific order of asynchronous operation that were fired paralellely. This was true on most platform and conditions, but not all the time. See: nodejs#18986 PR-URL: nodejs#19093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket. Backport-PR-URL: #20456 PR-URL: #18986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The test was flaky because it relied on a specific order of asynchronous operation that were fired paralellely. This was true on most platform and conditions, but not all the time. See: #18986 Backport-PR-URL: #20456 PR-URL: #19093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket. PR-URL: nodejs#18986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The test was flaky because it relied on a specific order of asynchronous operation that were fired paralellely. This was true on most platform and conditions, but not all the time. See: nodejs#18986 PR-URL: nodejs#19093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket. Backport-PR-URL: #20456 PR-URL: #18986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The test was flaky because it relied on a specific order of asynchronous operation that were fired paralellely. This was true on most platform and conditions, but not all the time. See: #18986 Backport-PR-URL: #20456 PR-URL: #19093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket. Backport-PR-URL: #20456 PR-URL: #18986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The test was flaky because it relied on a specific order of asynchronous operation that were fired paralellely. This was true on most platform and conditions, but not all the time. See: #18986 Backport-PR-URL: #20456 PR-URL: #19093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

http2: send error text in case of ALPN mismatch

2d102f4

Send a human-readable HTTP/1 response in case of an unexpected ALPN protocol. This helps with debugging this condition, since previously the only result of it would be a closed socket.

nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Feb 25, 2018

jasnell approved these changes Feb 25, 2018

View reviewed changes

devsnek approved these changes Feb 25, 2018

View reviewed changes

cjihrig approved these changes Feb 25, 2018

View reviewed changes

mcollina approved these changes Feb 25, 2018

View reviewed changes

lpinca approved these changes Feb 26, 2018

View reviewed changes

BridgeAR approved these changes Mar 1, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 1, 2018

mcollina closed this Mar 2, 2018

richardlau mentioned this pull request Mar 2, 2018

src: add incr/decr operators for Reference #19083

Closed

2 tasks

richardlau mentioned this pull request Mar 2, 2018

process: unify error message from chdir() errors #19088

Closed

3 tasks

mcollina mentioned this pull request Mar 2, 2018

http2: fix flaky test-http2-https-fallback #19093

Closed

4 tasks

addaleax deleted the http2-alpn-mismatch-text branch March 2, 2018 21:41

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

bnoordhuis mentioned this pull request Aug 17, 2020

http2: cannot negotiate ALPN besides http/1.1 #26835

Closed

http2: send error text in case of ALPN mismatch by addaleax · Pull Request #18986 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: send error text in case of ALPN mismatch - #18986

http2: send error text in case of ALPN mismatch#18986
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:http2-alpn-mismatch-text

addaleax commented Feb 25, 2018

Uh oh!

addaleax commented Feb 25, 2018

Uh oh!

mcollina left a comment

Uh oh!

mcollina commented Mar 2, 2018

Uh oh!

richardlau commented Mar 2, 2018

Uh oh!

mcollina commented Mar 2, 2018

Uh oh!

mcollina commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

http2: send error text in case of ALPN mismatch by addaleax · Pull Request #18986 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: send error text in case of ALPN mismatch - #18986

http2: send error text in case of ALPN mismatch#18986addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:http2-alpn-mismatch-textaddaleax/node:http2-alpn-mismatch-textCopy head branch name to clipboard

Conversation

addaleax commented Feb 25, 2018

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Feb 25, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 2, 2018

Uh oh!

richardlau commented Mar 2, 2018

Uh oh!

mcollina commented Mar 2, 2018

Uh oh!

mcollina commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

http2: send error text in case of ALPN mismatch#18986
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:http2-alpn-mismatch-text