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

Fixes issue #952 - did not write everything to output stream #1010

Merged
merged 2 commits into from
Nov 26, 2020

Conversation

cocytus
Copy link
Contributor

@cocytus cocytus commented Nov 26, 2020

Refit might not write everything to the outgoing http stream depending on buffer size and the total amount of data it should sent. This PR fixes that.

Should I also create a PR for 5.2.1, since it's a pretty critical bug, I'd think?

Unsure how to create a good unit/integration test for this - any help would be appreciated.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

@dnfadmin
Copy link

dnfadmin commented Nov 26, 2020

CLA assistant check
All CLA requirements met.

@cocytus
Copy link
Contributor Author

cocytus commented Nov 26, 2020

Note: This also uses WriteAsync as opposed to Write(), which would block the calling thread until everything was written.

@cocytus
Copy link
Contributor Author

cocytus commented Nov 26, 2020

Should I instead create a PR to 5.2.1 tag?

@clairernovotny
Copy link
Member

clairernovotny commented Nov 26, 2020

Thanks! I can cherry pick the result to that branch once it's merged into main.

I would like to get a failing/fixed test that shows this -- the key is figuring out what the interface/datatype/payload was that was causing this? Something that was too big? We serialize a known array of data or a file in the test if that's it.

There's a mocking infrastructure that lets us check the contents of what the server receives, similar to this:
https://github.com/reactiveui/refit/blob/main/Refit.Tests/RestService.cs#L1406-L1429

GitHub
The automatic type-safe REST library for .NET Core, Xamarin and .NET. Heavily inspired by Square's Retrofit library, Refit turns your REST API into a live interface. - reactiveui/refit

@cocytus cocytus force-pushed the fix_netstd21_write_main branch from 104bbc9 to d94f5eb Compare November 26, 2020 13:34
@cocytus
Copy link
Contributor Author

cocytus commented Nov 26, 2020

Test added in separate commit. It fails before last commit is added.

@cocytus cocytus force-pushed the fix_netstd21_write_main branch from d94f5eb to f01a96b Compare November 26, 2020 13:58
@clairernovotny clairernovotny merged commit 4c2b17e into reactiveui:main Nov 26, 2020
clairernovotny added a commit that referenced this pull request Nov 26, 2020
Fixes issue #952 - did not write everything to output stream
@clairernovotny
Copy link
Member

After cherry-picking the fix, there's a failing test on the rel/v5.2 branch: CanSerializeBigData. The BigData member is null. Any chance you could take a quick look so we can get fix out for 5.2?

@cocytus cocytus deleted the fix_netstd21_write_main branch November 26, 2020 15:57
@cocytus
Copy link
Contributor Author

cocytus commented Nov 26, 2020

Created PR against 5.2 branch.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants