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

More informative error when connecting to unsupported HTTP/2 end-point #2043

Closed
Songkeys opened this issue Apr 4, 2023 · 6 comments · Fixed by #2055
Closed

More informative error when connecting to unsupported HTTP/2 end-point #2043

Songkeys opened this issue Apr 4, 2023 · 6 comments · Fixed by #2055
Labels
enhancement New feature or request good first issue Good for newcomers Status: help-wanted This issue/pr is open for contributions

Comments

@Songkeys
Copy link
Contributor

Songkeys commented Apr 4, 2023

Bug Description

node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

HTTPParserError: Expected HTTP/
    at Parser.execute (/Users/songkeys/GitHub/playground/node_modules/undici/lib/client.js:572:15)
    at Parser.readMore (/Users/songkeys/GitHub/playground/node_modules/undici/lib/client.js:516:12)
    at TLSSocket.onSocketReadable (/Users/songkeys/GitHub/playground/node_modules/undici/lib/client.js:940:10)
    at TLSSocket.emit (node:events:513:28)
    at emitReadable_ (node:internal/streams/readable:590:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:81:21) {
  code: 'HPE_INVALID_CONSTANT',
  data: '\x00\x00\x18\x04\x00\x00\x00\x00\x00\x00\x01\x00\x00\x10\x00\x00\x03\x00\x00\x00\x01\x00\x05\x00\x00@\x00\x00\x06\x00\x00\x1F@'
}

Node.js v19.3.0

Reproducible By

import { request } from "undici";

request("https://api.push.apple.com");

Environment

Node.js v19.3.0

undici@5.21.0

Additional context

This error only occurs on this specific endpoint.

@Songkeys Songkeys added the bug Something isn't working label Apr 4, 2023
@targos
Copy link
Member

targos commented Apr 4, 2023

The error is not very informative, but I think this is expected because that endpoint is HTTP/2, not HTTP/1.1

@Songkeys
Copy link
Contributor Author

Songkeys commented Apr 4, 2023

Oh I just realized the API is HTTP/2 and undici only supports HTTP/1.1 so far. You were right. The issue might be the uninformative error message then.

@ronag ronag added enhancement New feature or request and removed bug Something isn't working labels Apr 4, 2023
@ronag ronag changed the title HTTPParserError: Expected HTTP/ on a very minimal example More informative error when connecting to unsupported HTTP/2 end-point Apr 4, 2023
@ronag ronag added the Status: help-wanted This issue/pr is open for contributions label Apr 4, 2023
@mcollina
Copy link
Member

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina mcollina added the good first issue Good for newcomers label Apr 10, 2023
@Songkeys
Copy link
Contributor Author

@mcollina Yes. I'm interested. Give me some time :)

@Songkeys
Copy link
Contributor Author

Songkeys commented Apr 10, 2023

@mcollina This looks more tough than I thought.

I tried to compare the first chunk read from the socket with http2 preface:

const HTTP2_PREFACE = 'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n';
const http2PrefaceBuffer = Buffer.from(HTTP2_PREFACE, 'utf8');
return chunk.slice(0, http2PrefaceBuffer.length).equals(http2PrefaceBuffer);

but the chunk doesn't seem to make any sense. Take the API response in this issue as an example, it's: "\x00\x00\x18\x04\x00\x00\x00\x00\x00\x00\x01\x00\x00\x10\x00\x00\x03\x00\x00\x00\x01\x00\x05\x00\x00@\x00\x00\x06\x00\x00\x1F@".

I also tried to use socket.alpnProtocol to tell if the server is h2. but it always returns false.

Any instruction?


edit: do we need to actually tell if the server is h2 or just tell user that the http parsing is wrong and the server might be http2 with just changing the error message in HTTPParserError?

@mcollina
Copy link
Member

I think we can just mention that we expected HTTP/1.1 and we didn't get it (leaving to the reader to figure out what protocol was on the wire).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers Status: help-wanted This issue/pr is open for contributions
Projects
None yet
4 participants