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

Multipart form files only accept seekable streams #1398

Closed
ordinaryorange opened this issue Oct 28, 2021 · 2 comments
Closed

Multipart form files only accept seekable streams #1398

ordinaryorange opened this issue Oct 28, 2021 · 2 comments

Comments

@ordinaryorange
Copy link
Contributor

I've a situation where I need to pass in a PipeReader stream for use a MiltiPartItem form files for a POST request.

However I notice the current writeMultipart and CombinedStream implementations lean on the Stream.Length property which is where things fail.

The Length property assumes provided streams are seekable, which PipeReader (and others) are not.

In my testing I've removed all the Stream.Lengh references, (which also means the CombinedStream cant calculate it's length) and removed the assignment of length during request building.

Requests are still issued to and accepted by the server, so it initially seems ok to permit non seekable streams

Thoughts

  • This is using the current WebRequest implementation, and I've no idea if this will work in HttpClient Use System.Net.Http.HttpClient #1392
  • I'm unsure if it is a requirement for a POST request to have a Length.
  • Are non seekable streams for multipart items even a valid idea for inclusion
  • Anything else I've overlooked

Thoughts please, as to if this would be a valid inclusion ?

@ordinaryorange
Copy link
Contributor Author

I've done an implementation and tests here

Main points

  • I've added all the checks for non seekable streams where the content length cannot be determined up front.

  • I've also refactored the CombinedStream type to better report its Length and CanSeek implementations to be closer to the expected for Stream behaviour (even though CombindStream is totally internal)

  • And refactored the calculation of content length to be as light as possible with the IEnumerator on the incoming seq. (This was something I needed specifically for my passing of a BlockingCollection but should work with all others)

All tests pass, and implementation is working well in my app

Can someone review and let me know if ok for a PR ?

@cartermp
Copy link
Collaborator

@ordinaryorange Seems good enough to send as a PR to me! Will gladly review it.

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

No branches or pull requests

2 participants