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

Removed unnecessary 'static & Unpin bounds from body generic #649

Merged
merged 1 commit into from Dec 12, 2022

Conversation

gtsiam
Copy link
Contributor

@gtsiam gtsiam commented Nov 27, 2022

Motivation

When proxying large requests, I want to be able to pass the request body straight through my server without loading all of it it in memory at once. This is currently very difficult due to the (seemingly) arbitrary 'static bound on Hyper's body generic. And so, I want to remove the 'static bounds for hyper 1.0, but before I can do that I need to remove them from here. (Will this require an h2 release before I can make the hyper patch?)

Changes

@seanmonstar
Copy link
Member

It looks like the change here is actually quite small (the clippy stuff seems unrelated and could be a separate PR). I can't remember why the static bounds are there in the first place...

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Clippy changes like sean said should go in a separate PR but overall lgtm for the bound changes.

.gitignore Outdated Show resolved Hide resolved
@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 1, 2022

the clippy stuff seems unrelated and could be a separate PR

I still have a pr sitting in warp for more than a year, so I was expecting this to take ages too. But I'll keep it in mind for future contributions to hyper. Probably more effort than it's worth splitting it now. (But I can split it if it's necessary)

@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 1, 2022

On second thought I'll just make it a separate pr, no need for the extra back and forth

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

If CI is happy then LGTM! This should be a non-breaking change as well I believe.

@seanmonstar
Copy link
Member

seanmonstar commented Dec 1, 2022

I still have a pr sitting in warp for more than a year, so I was expecting this to take ages too.

Sorry! I haven't been able to keep up with warp. My attention has been sucked up with hyper and its dependencies.

@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 1, 2022

I still have a pr sitting in warp for more than a year, so I was expecting this to take ages too.

Sorry! I haven't been able to keep up with warp. My attention has been sucked up with hyper and its dependencies.

It's understandable, there's only so much time in a day. Regardless, I don't use warp much anymore. It's just that warp & mime is what shaped my expectation that this would be sitting here forever and pushed me to dump everything in one pr (not making it very easy to review I'll admit) - should've looked at other PRs a bit more closely before doing that.

That said, it might be worth looking into getting some maintainers for your undead projects or mark them as such at some point.

@LucioFranco LucioFranco merged commit c1ce37e into hyperium:master Dec 12, 2022
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.

None yet

3 participants