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

Fixed deadlock when serializing content #590

Merged
merged 1 commit into from Dec 5, 2018
Merged

Fixed deadlock when serializing content #590

merged 1 commit into from Dec 5, 2018

Conversation

stevewgh
Copy link
Contributor

@stevewgh stevewgh commented Dec 5, 2018

This fixes an issue that was discovered after pull request #571 was merged to master (MyGet version 4.6.66).

A request which contains a body that should be serialized (and is not a multi-part request) will deadlock in the PushStreamContent at:

await serializeToStreamTask.Task;

The PushStreamContent content type provides a stream to write to, but in order for it to complete the request it blocks until the stream is closed. The original implementation did this implicitly because a StreamWriter was used within a using block:

ret.Content = new PushStreamContent((stream, _, __) =>
{
using (var writer = new JsonTextWriter(new StreamWriter(stream)))
{
serializer.Serialize(writer, param);
}
},
new MediaTypeHeaderValue("application/json") { CharSet = "utf-8" });

I've added additional tests to make sure that the request doesn't block and it also checks the other flow through the RequestBuilderImplementation class where the request is not a multi-part request. I also fixed a typo in existing tests where a wrong exception was thrown.

@clairernovotny clairernovotny merged commit 846f5f8 into reactiveui:master Dec 5, 2018
@stevewgh stevewgh deleted the custom-serializer-bug branch December 8, 2018 10:43
@lock lock bot locked and limited conversation to collaborators Jun 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants