Skip to content

Synchronously finalize headers #359

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

Merged
merged 3 commits into from
Jan 3, 2020
Merged

Synchronously finalize headers #359

merged 3 commits into from
Jan 3, 2020

Conversation

natebosch
Copy link
Member

Fixes #351

An async* method is not started until after the stream has a listener,
if a caller reads the headers before listening to the stream, they would
see the value before they have been set. Move the header assignment and
other finalization work to the wrapping method and keep the async*
method handling only the stream content.

Fixes #351

An `async*` method is not started until after the stream has a listener,
if a caller reads the headers before listening to the stream, they would
see the value before they have been set. Move the header assignment and
other finalization work to the wrapping method and keep the `async*`
method handling only the stream content.
Copy link
Contributor

@jakemac53 jakemac53 left a comment

Choose a reason for hiding this comment

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

It is likely worth writing a regression test for this if possible

@natebosch
Copy link
Member Author

It is likely worth writing a regression test for this if possible

Yup, I'm trying to figure out the best way to do that.

@natebosch
Copy link
Member Author

I added a test that fails before the change and passes after.

I put the test in with the IO client tests since it's really an issue of the interaction between the IO client and MultipartRequest.

PTAL

@natebosch natebosch merged commit 29d3262 into master Jan 3, 2020
@natebosch natebosch deleted the sync-finalize-headers branch January 3, 2020 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MultipartRequest does not contain Content-Type header
3 participants