search

go-resty / resty

Simple HTTP and REST client library for Go
MIT License
10.13k stars 710 forks source link

Retry doesn't work with FileReader and MultipartFields #334

Closed phsym closed 2 weeks ago

phsym commented 4 years ago

Hi, First, thank you for this excellent library.

Description

I'm trying to send data using FileReaderand MultipartFields, but the server I'm sending those data to is a bit crappy, and sometimes drops the connections. Unfortunately I have no control on this server. I configured the resty client to use the retry mechanisms, which works well for almost all my requests, except those with FileReaderand MultipartFields. Each retry attempt is having an EOFerror because the io.Reader has been consumed already.

I believe this issue will happen also with Request.Body implementing io.Reader, but I have not checked.

Possible fix

A possible fix would be to check if the readers implement io.Seeker, and seek back to the beginning. But maybe we should consider the case where a reader is not read from the beginning, thus seeking to offset 0 would be problematic. What about keeping the number of bytes read, and seek back to the original offset ?

Other alternative, probably more efficient, would be to keep Request.bodyBuf between attempts, not trying to create it each time, then releasing this buffer only on success.

Workaround

In the meantime, as a workaround, I'm writing my data to a temporary file, and I use the SetFilemethod.

jeevatkm commented 4 years ago

@phsym I believe your understanding is correct. Resty keeps complete request in the memory before it sends to the server, this approach has downside on large request body scenarios, that's why io.Reader introduced and resty submits the body handling to underlying Go HTTP client if its an io.Reader.

I'm not sure, keeping Request.bodyBuf around all retry. what will be the side effect? I have to try out and see.

I will have to think about, how to further improve the request body handling for various use cases supported by resty. Right now I don't have an answer. I'm sorry!

jeevatkm commented 4 years ago

Just referencing an issue #309

LBeernaertProton commented 1 year ago

We recently ran into this issue, I have made a fix in a fork which suits our current needs. Let me know if you would be interested in a pull request based of those changes.

Code is available here.

jeevatkm commented 2 weeks ago

This has been taken care of as part of #879, and Resty automatically seeks if the provided reader has an io.ReadSeeker interface.