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

feat: check maxHeadersSize on client instantiation and not on Parser instantion #3654

Merged
merged 5 commits into from
Oct 6, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Sep 30, 2024

I guess this would fix 3652 . I am less inclined to download NW.js and figure out how it works. Imho validation of maxHeadersSize should not be done in Parser construction but in the client instantiation.

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

Adding the extra step of validation on dispatcher/client lgmt, but I believe the issue will prevail.

The problem roots to the defaults. If no maxHeaderSize passed and if the runtime doesn't populate [(node:http).maxHeaderSize](https://nodejs.org/api/http.html#httpmaxheadersize), there won't be value to check against it.

I believe the check on client-h1 aimed to avoid that exact scenario.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 1, 2024

Yeah, I guess we can consider this an improvement and not a fix. Unfortunately i can somehow not remove the link to that issue. If you approve I would merge it and then reopen the linked issue.

@Uzlopak Uzlopak changed the title fix: check maxHeadersSize on client instantiation and not on Parser instantion feat: check maxHeadersSize on client instantiation and not on Parser instantion Oct 1, 2024
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 3, 2024

@metcoder95

I still think that we dont need that check in the parser. Also why should the parser check if the attribute on the client is valid? Actually hints it to be a validation, that has to happen in the Client API. But you are right, we need to be stricter.

PTAL

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 3, 2024

Or wait... hmm, wont it mean, that you can set maxHeaderSize to 0 meaning any header size?

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

LGTM,

I was mostly meaning the way we get the default max header size; which you already addressed within the lib/dispatcher/client

@metcoder95
Copy link
Member

Just tests seems not happy

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 5, 2024

@metcoder95

Tests should now be happy. :D

Also I think that the check in parser is not necessary anymore, because it is handled properly in client already.

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

lgtm

@Uzlopak Uzlopak merged commit 01abc4f into main Oct 6, 2024
38 checks passed
@Uzlopak Uzlopak deleted the fix-3653 branch October 6, 2024 13:20
flakey5 pushed a commit to flakey5/undici that referenced this pull request Oct 8, 2024
…instantion (nodejs#3654)

* fix: check maxHeadersSize on client instantiation and not on Parser instantion

* ensure that http.maxHeadersize is valid

* fail early, fail fast

* fix

---------

Co-authored-by: Carlos Fuentes <me@metcoder.dev>
Copy link
Contributor

The backport to v6.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-3654-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 01abc4fc601edba0f5ec7d2cb2388e50e006d3cd
# Push it to GitHub
git push --set-upstream origin backport-3654-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.x

Then, create a pull request where the base branch is v6.x and the compare/head branch is backport-3654-to-v6.x.

@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants