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 vs http1 support seems backwards #7

Open
2 tasks done
moonmeister opened this issue Nov 3, 2022 · 14 comments
Open
2 tasks done

http2 vs http1 support seems backwards #7

moonmeister opened this issue Nov 3, 2022 · 14 comments

Comments

@moonmeister
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

So, in everything I've read, but most importantly, the Chrome docs, Early Hints are only supported on http2 and 3. But, this plugin throws an error if I try to set Fastify to use http2.

Is this not backwards? If correct as is, what's the reason we've disabled this on http/2? Thanks all!

@climba03003
Copy link
Member

Is this not backwards?

It is the choice from Chromium and it does not means Early Hints is not supported in HTTP/1.1.

What's the reason we've disabled this on http/2?

I believe is the complexity of directly write to socket for HTTP/2.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 4, 2022

Also we expect that fastify is behind a reverse proxy like Ingress/traefik which resolves a http2 request to http1.1 and vice versa.

Maybe we can transplant http2 from nodejs sourcecode to our plugin?

@climba03003
Copy link
Member

climba03003 commented Nov 4, 2022

Maybe we can transplant http2 from nodejs sourcecode to our plugin?

Yes, we can use reply.raw.stream for the reply.
Details on https://nodejs.org/api/http2.html#http2streamadditionalheadersheaders

@mcollina
Copy link
Member

mcollina commented Nov 4, 2022

I think we should just use the native functionality: https://nodejs.org/api/http2.html#responsewriteearlyhintslinks.

@climba03003
Copy link
Member

I think we should just use the native functionality: https://nodejs.org/api/http2.html#responsewriteearlyhintslinks.

We need to provide a compatible layer for node@14 and node@16.
Patching to use additionalHeaders and .socket are needed.

And, if the native one is available, for example node@18.11.0. We should absolutely go into that.

@jsumners
Copy link
Member

jsumners commented Nov 4, 2022

Also we expect that fastify is behind a reverse proxy like Ingress/traefik which resolves a http2 request to http1.1 and vice versa.

That's not necessarily true. A reverse proxy can still use http/2 to communicate to the backend services.

Maybe we can transplant http2 from nodejs sourcecode to our plugin?

That would be a maintenance nightmare.

@climba03003
Copy link
Member

That would be a maintenance nightmare.

Not really re-invented the wheel, http2 already provide a method called additionalHeaders. We can directly pass the header with additional :status.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 4, 2022

My assessment:

  • modify formatEntry to not add "Link:" when http2
  • concat the early hints into message not with CRLF but with a comma, no trailing comma
  • use additionalHeaders instead of write on reply.raw

I am wondering how we can test http2 because undici does not support http2

@climba03003
Copy link
Member

I am wondering how we can test http2 because undici does not support http2

Use built-in net or http2 package.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 4, 2022

created #8 for discussing http2 implementation

@lilouartz
Copy link

Looks like the PR was closed without resolution :-(

@climba03003
Copy link
Member

Looks like the PR was closed without resolution :-(

Since, Node.js already implemented the writeEarlyHints in both http and http2.
You should use the native function instead of relying this project (unless you are in old Node.js version).

@lilouartz
Copy link

ah, would be useful to document this somewhere.

@mcollina
Copy link
Member

fastify/fastify#5479

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

No branches or pull requests

6 participants