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

Enable h2c support #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enable h2c support #3

wants to merge 1 commit into from

Conversation

kevinmcconnell
Copy link
Collaborator

No description provided.

@skulos
Copy link

skulos commented Jan 7, 2025

Any updates on this PR? For GCP we needed H2C so I forked this repo and added H2C similar to this PR. Would like to use the gem instead of dragging my repo in and compiling it to run with the Rails app on GCP.

@skulos
Copy link

skulos commented Jan 14, 2025

@kevinmcconnell are there any updates on this PR?

@skulos
Copy link

skulos commented Jan 20, 2025

Are there any updates on this PR?
@kevinmcconnell @3v0k4

@3v0k4
Copy link
Contributor

3v0k4 commented Jan 20, 2025

Hey @skulos, you'll need to wait for Kevin on this one.

Sorry, I cannot help more 🙂

@skulos
Copy link

skulos commented Feb 3, 2025

@kevinmcconnell any updates on this PR?

@kevinmcconnell
Copy link
Collaborator Author

Hi @skulos, sorry for the slow response on this one. And thanks for the nudges :)

One possible side effect of this is the fact that the entire request will be read into memory during an h2c upgrade. We have a MAX_REQUEST_BODY that can be used to guard against maliciously-large requests, but this does mean that supporting h2c (at least with this implementation) makes setting a limit more necessary. And it's hard to set a good default on that, since it's so application-specific.

So I'm thinking we could initially ship this behind an opt-in flag, like H2C_ENABLED. People who need it can enable it, along with setting an appropriate request size limit. People who don't need it (which is probably most folks) won't be affected. What do you think?

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

Successfully merging this pull request may close these issues.

3 participants