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

Ensure chunk_size is properly set when chunking responses #1336

Merged
merged 2 commits into from Sep 19, 2022

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Sep 19, 2022

Closes #1335.

The test added in 4352a85 does not trigger even without the fix, so still not entirely sure how to reliably trigger this bug.

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Looks good to me, but as mentioned in #1335, it may not be an empty body that triggered the issue to begin with, but rather some form of response generation time out?
Difficult to test - however, it's always nice with some extra edge-cases tested, I suppose?

Thanks for the quick response and fix!

@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #1336 (3233ee0) into master (3387a8a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1336      +/-   ##
==========================================
- Coverage   91.46%   91.45%   -0.01%     
==========================================
  Files          72       72              
  Lines        4368     4366       -2     
==========================================
- Hits         3995     3993       -2     
  Misses        373      373              
Flag Coverage Δ
project 91.45% <100.00%> (-0.01%) ⬇️
validator 90.86% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/server/middleware.py 94.87% <100.00%> (-0.07%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ml-evs
Copy link
Member Author

ml-evs commented Sep 19, 2022

Looks good to me, but as mentioned in #1335, it may not be an empty body that triggered the issue to begin with, but rather some form of response generation time out? Difficult to test - however, it's always nice with some extra edge-cases tested, I suppose?

Thanks for the quick response and fix!

Yeah, I've just played around with artificial pauses in the dummy endpoint but can't trigger it (and couldn't find a way within starlette to just drop the request). Do you want a hotfix release for this?

@ml-evs ml-evs added bug Something isn't working server Issues pertaining to the example server implementation labels Sep 19, 2022
@ml-evs ml-evs merged commit f9f7080 into master Sep 19, 2022
@ml-evs ml-evs deleted the ml-evs/close_#1335 branch September 19, 2022 11:39
@CasperWA
Copy link
Member

Yeah, I've just played around with artificial pauses in the dummy endpoint but can't trigger it (and couldn't find a way within starlette to just drop the request). Do you want a hotfix release for this?

Weird. It might be something different again then? At least the code base change is a good one either way.

If you're offering a hotfix release I'm game :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server Issues pertaining to the example server implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnboundLocalError - chunk_size is not always set in middleware method
2 participants