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

[C#] Stream file uploads instead of copy all bytes into memory #2819

Open
hacki11 opened this issue May 10, 2016 · 17 comments
Open

[C#] Stream file uploads instead of copy all bytes into memory #2819

hacki11 opened this issue May 10, 2016 · 17 comments

Comments

@hacki11
Copy link
Contributor

hacki11 commented May 10, 2016

ApiClient::ParameterToFile will copy complete file into ram before upload to rest service.

what about streaming a file to avoid OutOfMemory Exceptions on large files like:

public FileParameter ParameterToFile(string name, Stream stream)
        {
            var fp = new FileParameter
            {
                Writer = s =>
                {
                    using (StreamReader file = new StreamReader(stream))
                    {
                        file.BaseStream.CopyTo(s);
                    }
                },
                Name = name
            };


            if (stream is FileStream)
                fp.FileName = Path.GetFileName(((FileStream) stream).Name);
            else
                fp.FileName = "no_file_name_provided";
            return fp;
        }

current implementation:
ApiClient.mustache

if (stream is FileStream)
                return FileParameter.Create(name, ReadAsBytes(stream), Path.GetFileName(((FileStream)stream).Name));
            else
                return FileParameter.Create(name, ReadAsBytes(stream), "no_file_name_provided");
@wing328
Copy link
Contributor

wing328 commented May 10, 2016

@hacki11 thanks for the suggestion. Do you have cycle to submit a PR with the suggested change?

Ref: https://github.com/swagger-api/swagger-codegen/blob/master/CONTRIBUTING.md

@hacki11
Copy link
Contributor Author

hacki11 commented May 11, 2016

Unfortunately, RestSharp 105.1.0 loads the complete body into memory before send.
The current RestSharp version has an issue not setting the Content-Length attribute where the rest server gets an eof exception....

@wing328
Copy link
Contributor

wing328 commented May 11, 2016

@hacki11 are you referring to this issue? restsharp/RestSharp#814

Roughly how large is your file?

@hacki11
Copy link
Contributor Author

hacki11 commented May 11, 2016

@wing328 i did not know there is already an issue - nice investigation!

it can easily grow to several hundred MB to 1 GB.
Due to migrating my project to C# (java before) the problem arises. Java can stream perfectly with jaxrs.

@wing328
Copy link
Contributor

wing328 commented May 11, 2016

@hacki11 you may want to try compiling the latest version (unofficial release) as another user has confirmed that it did address some of the issue with file upload: #2175

@hacki11
Copy link
Contributor Author

hacki11 commented May 11, 2016

i compiled the RestSharp master and the EOF Error on server side due to missing Content-Length is fixed now.
But the execute function of RestSharp seems to still load the whole stream into memory before streaming the content to the server.
There was already a stackoverflow discussion SO but it did not work for me

@wing328
Copy link
Contributor

wing328 commented May 14, 2016

If you perform a debug, did you hit the following line mentioned in the SO comment?

https://github.com/restsharp/RestSharp/blob/62335a677c92ad45d9dc3214ff6765b597d8642f/RestSharp/Http.Sync.cs#L226

@hacki11
Copy link
Contributor Author

hacki11 commented May 15, 2016

yes, I hit that line and it uses the Action of the FileParameter Writer Property.
After RestSharp ran the line above file.BaseStream.CopyTo(s); the process memory fills up with the amount of the file that is sent (tested with 250mb file).
i did assume that with this stream, the file will not be loaded completely into the ram.
i am not sure where the issue lies exactly.

@wing328
Copy link
Contributor

wing328 commented May 17, 2016

@hacki11 I would suggest 2 things:

  1. test with just RestSharp with a simple script to upload large files to see if you're experiencing the same so as so isolate the problem with RestSharp

  2. upload a larger file (e.g. 2GB) to make sure you get the out of memory exception (so as to confirm the RestSharp is not smart enough to stream large files by not loading the whole file into memory)

@jimschubert
Copy link
Contributor

It looks like RestSharp supports file streaming via WriteMultipartFormData. You'd have to set AlwaysMultipartFormData = true to hit this case:

RestRequest request = new RestRequest("/upload", Method.POST) 
{ 
  AlwaysMultipartFormData = true 
};

@wing328
Copy link
Contributor

wing328 commented Jun 20, 2016

@hacki11 did you have a chance to review the feedback from @jimschubert and give it a try?

@hacki11
Copy link
Contributor Author

hacki11 commented Jun 28, 2016

it doesn't make a difference setting AlwaysMultipartFormData = true because the caller of the WriteMultipartFormData function checks the HasFiles flag which is true in my case:

Http.Sync.cs:
if (this.HasFiles || this.AlwaysMultipartFormData)
{
    this.WriteMultipartFormData(requestStream);
}

For completeness i set the AlwaysMultipartFormData = true, but the process consumed 1GB memory like the file to be uploaded.
image

Debugging makes me wonder where the issue lies.
WriteMultipartFormData tries to write the stream to the webrequest stream (seems correct to me):
image

This function calls my Writer Action in the FileParameter object:
image
After executing the CopyTo function memory fills up


After some tests i end up with a OutOfMemory Exception already at this line using (Stream requestStream = webRequest.GetRequestStream()) but can't tell really why.

@jimschubert
Copy link
Contributor

@hacki11 thanks for your investigation work.

Looks like you'd get the exception on that line because ParameterToFile is flushing the stream.

Do you have a test project repo you used for investigation, or were you testing against your codebase?

@wing328 wing328 modified the milestones: v2.3.0, v2.2.0 Jul 7, 2016
@wing328 wing328 modified the milestones: v2.2.1, v2.2.2, Future Aug 8, 2016
@chenzhekl
Copy link

Seems to be related to this restsharp/RestSharp#889

@wing328
Copy link
Contributor

wing328 commented Dec 11, 2017

@chenzhekl thanks for the pointer. You may be interested in this task as well: #7012

@nevergiveupc
Copy link

Is this problem solved or not? @hacki11 @wing328

@alexeyzimarev
Copy link

Don't use FileParameter. Use AddFile overload that accepts the stream writer delegate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants