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

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

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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.
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Feb 25, 2018
@addaleax
Copy link
Member Author

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018
@mcollina
Copy link
Member

mcollina commented Mar 2, 2018

Landed as d1bc6f0.

@mcollina mcollina closed this Mar 2, 2018
mcollina pushed a commit that referenced this pull request Mar 2, 2018
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@richardlau
Copy link
Member

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)
  ...

@mcollina
Copy link
Member

mcollina commented Mar 2, 2018

@mcollina
Copy link
Member

mcollina commented Mar 2, 2018

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

mcollina added a commit to mcollina/node that referenced this pull request Mar 2, 2018
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
@addaleax addaleax deleted the http2-alpn-mismatch-text branch March 2, 2018 21:41
mcollina added a commit that referenced this pull request Mar 3, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Mar 5, 2018
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants