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

fetch support for HTTP/2 by default #2750

Closed
mctrafik opened this issue Feb 12, 2024 · 21 comments
Closed

fetch support for HTTP/2 by default #2750

mctrafik opened this issue Feb 12, 2024 · 21 comments

Comments

@mctrafik
Copy link

What is the problem this feature will solve?

This will get node's implementation of fetch closer to how it behaves in the browser.

What is the feature you are proposing to solve the problem?

Support protocol upgrade similar to how fetch-h2 package does. It's weird to try to use a third-party package that uses node's internal http2 package under the hood.

What alternatives have you considered?

I am now using fetch-h2 package, but much like got-fetch or node-fetch I fear it will fall out of spec and not evolve with the ecosystem.

@aduh95
Copy link
Contributor

aduh95 commented Feb 12, 2024

Duplicate of #399. AFAICT it has already been implemented.

@aduh95 aduh95 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 12, 2024
@mctrafik
Copy link
Author

@aduh95 Can you elaborate? Does nodeJs use undici? If yes, in what version is this supported? I'm using Node 20, which is latest LTE version and fetch there doesn't support H2.

@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2024

Does nodeJs use undici?

Correct (it is also maintained by us) https://github.com/nodejs/node/blob/109ab0a89cdeff5717a2a16edd27bafecc104cf6/doc/contributing/maintaining/maintaining-http.md#L54-L56

You can check what version of Undici your build is using in process.versions.undici.

I'm using Node 20, which is latest LTE version and fetch there doesn't support H2.

Any chance you could send a repro? So we can reopen this issue and transfer it to the Undici repo.

@targos
Copy link
Member

targos commented Feb 13, 2024

I don't know much about HTTP/2, but here's an example that works in a browser (do it from the website itself to avoid CORS issue), while it fails in Node.js:

fetch('https://api.sandbox.push.apple.com').then(r => r.text()).then(console.log)

It's not easy to find a public URL that only works with HTTP/2.

@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2024

Created a minimal repro:

import { createServer, constants } from 'node:http2';
import { once } from 'node:events';

const {
  HTTP2_HEADER_STATUS,
  HTTP2_HEADER_CONTENT_TYPE,
} = constants;

const server = createServer();

server.on('stream', (stream, headers) => {
  stream.respond({
    [HTTP2_HEADER_STATUS]: 200,
    [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain; charset=utf-8',
    'Access-Control-Allow-Origin': '*',
  });
  stream.write('hello world');
  stream.end();
});


server.listen(3000);
await once(server, 'listening');

const response = await fetch('http://localhost:3000/');

console.assert(response.ok, response.status);
console.log(await response.text());

server.close();

I can confirm the above code works with Deno and fails with the latest main.

@aduh95 aduh95 reopened this Feb 13, 2024
@aduh95 aduh95 transferred this issue from nodejs/node Feb 13, 2024
@mcollina
Copy link
Member

HTTP/2 support in undici is experimental and not enabled by default. Note that we do not support H2, but only HTTP/2 (over TLS), due the necessary protocol selection.

To enable:

import { createServer, constants } from 'node:http2';
import { once } from 'node:events';
import { setGlobalDispatcher, Agent } from './index.js';

const {
  HTTP2_HEADER_STATUS,
  HTTP2_HEADER_CONTENT_TYPE,
} = constants;

setGlobalDispatcher(new Agent({
  allowH2: true
}));

fetch('https://api.sandbox.push.apple.com').then(r => r.text()).then(console.log)

I think we could enable it in the next major and see how it goes. Unfortunately it might be breaking in case of bugs.

@mcollina mcollina changed the title fetch support for HTTP/2 fetch support for HTTP/2 by default Feb 13, 2024
@metcoder95
Copy link
Member

I believe it should be fine as soon as we keep it as experimental though enabled by default when chosen by the server.

@mcollina
Copy link
Member

I believe it should be fine as soon as we keep it as experimental though enabled by default when chosen by the server.

Good Idea! Repeating it for everybody:

We want to enable HTTP/2 if it is the only protocol that the server advertises in the TLS exchange, but prefer HTTP/1.1 for everything else. 100% agreed.PR?

@mctrafik
Copy link
Author

mctrafik commented Feb 14, 2024

@mcolina Just to clraify: I'm using Http2SecureServer with allowHttp1: true to return an error on unexpectedProtocol. So sounds like my server would always return the error in this proposal.

Can this behavior be controlled by a flag?!

@mcollina
Copy link
Member

@mctrafik you can already turn on http2 with:

import { setGlobalDispatcher, Agent } from 'undici';

setGlobalDispatcher(new Agent({
  allowH2: true
}));

@metcoder95
Copy link
Member

I believe it should be fine as soon as we keep it as experimental though enabled by default when chosen by the server.

Good Idea! Repeating it for everybody:

We want to enable HTTP/2 if it is the only protocol that the server advertises in the TLS exchange, but prefer HTTP/1.1 for everything else. 100% agreed.PR?

SGTM 👍

@3x071c
Copy link

3x071c commented Mar 16, 2024

If anyone is looking for a way to enable H2 without requiring undici (which is currently not exposed as a node built-in), here's the trick:

global[Symbol.for("undici.globalDispatcher.1")] = new global[Symbol.for("undici.globalDispatcher.1")].constructor({
	allowH2: true,
});

Cheers

@hjr3
Copy link

hjr3 commented Apr 6, 2024

Note that we do not support H2, but only HTTP/2

What does this mean?

From RFC 9113:

The string "h2" identifies the protocol where HTTP/2 uses Transport Layer Security (TLS)

I am trying to understand the nuance of not supporting H2, but only HTTP/2.

@mcollina
Copy link
Member

mcollina commented Apr 7, 2024

I am trying to understand the nuance of not supporting H2, but only HTTP/2.

I meant H2C, which is the non-tls variant.

@langtutheky
Copy link

langtutheky commented Aug 1, 2024

global[Symbol.for("undici.globalDispatcher.1")] = new global[Symbol.for("undici.globalDispatcher.1")].constructor({
	allowH2: true,
});

@3x071c Could you please provide the TypeScript equivalent of this code, the compiler complains Element implicitly has an 'any' type because expression of type 'symbol' can't be used to index type 'typeof globalThis'. Much appreciated.

@ntwcklng
Copy link

ntwcklng commented Aug 1, 2024

global[Symbol.for("undici.globalDispatcher.1")] = new global[Symbol.for("undici.globalDispatcher.1")].constructor({
	allowH2: true,
});

@3x071c Could you please provide the TypeScript equivalent of this code, the compiler complains Element implicitly has an 'any' type because expression of type 'symbol' can't be used to index type 'typeof globalThis'. Much appreciated.

use this:
;(global as any)[Symbol.for('undici.globalDispatcher.1')] = new (global as any)[ Symbol.for('undici.globalDispatcher.1') ].constructor({ allowH2: true, })

@langtutheky
Copy link

@ntwcklng the snippet does silent TypeScript compiler but apparently it does not work as @3x071c mentioned to enable H2 without requiring undici because (global as any)[Symbol.for('undici.globalDispatcher.1')] returns undefined. I am running node v20.2.0

@bmingles
Copy link

@mcollina Should the provided config work in node 20?

setGlobalDispatcher(new Agent({
  allowH2: true
}));

@mcollina
Copy link
Member

Yes

@metcoder95
Copy link
Member

As h2 won't be enable by default but its out of experimental, shall we close this issue?

@mcollina mcollina closed this as completed Dec 2, 2024
@mcollina
Copy link
Member

mcollina commented Dec 2, 2024

Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

10 participants