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

Avoid processing incoming bodies unnecessarily #527

Merged
merged 4 commits into from
Oct 10, 2024
Merged

Conversation

IvanUkhov
Copy link
Contributor

Previously, I defined the body to be Full<Bytes>, which meant that the response would have to be fully received inside the API functions. However, that seems wasteful if one wants to do some streaming processing of the incoming data. This PR switches over to http_body_util::combinators::BoxBody, which is what is recommended in cases of having to accommodate different response types.

@IvanUkhov IvanUkhov marked this pull request as draft October 10, 2024 14:28
@IvanUkhov IvanUkhov marked this pull request as ready for review October 10, 2024 15:51
Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your help with this, it's much appreciated!

Now that it seems that CI caught a legitimate issue that is way too easily ignored, let's merge this PR only after the CLI related tests are in their own jobs so they can be expected to fail officially (I think CI can also be told so) so we are back at 'green CI by default'.

That should make further possibly even sweeping changes possible without adding unintended breakage.

src/generator/templates/Cargo.toml.mako Show resolved Hide resolved
@IvanUkhov
Copy link
Contributor Author

Oh, was not paying attention to the CLIs. I will take a look.

@Byron
Copy link
Owner

Byron commented Oct 10, 2024

The CLIs don't even get to fail as there is a legitimate failure elsewhere. Maybe it was previously introduced and nobody noticed, but either way, that just shows it's a problem to live with red CI. What I can live with is red CI for CLIs, once separate, but think that ideally it should then also be marked to allowed failure.

@IvanUkhov
Copy link
Contributor Author

Yeah, I think that one is a legitimate failure that should be eliminated. If I had noticed, I would not have let it in.

@IvanUkhov
Copy link
Contributor Author

I actually misread the error. It was not about the CLIs; it was about the APIs with the default features disabled. It should be resolved now.

@Byron
Copy link
Owner

Byron commented Oct 10, 2024

Thanks a lot, it now looks like a 'good failure'.

Do I understand correctly that you are not interested in splitting the CI jobs between API and CLI to prevent such issues in future?

@IvanUkhov
Copy link
Contributor Author

Sure! Let me do that.

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks again, particularly about the improvements to CI!
Now I am also curious if 'expects to fail' is something that CI can do, but then again, I still have hopes that the CLIs can be made to work or that those that aren't working can be excluded one day.

@Byron Byron merged commit fcd59df into Byron:main Oct 10, 2024
5 of 6 checks passed
@IvanUkhov IvanUkhov deleted the body branch October 11, 2024 06:10
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.

2 participants