Skip to content

added support for streaming as per issue #5 #7

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

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

Driedas
Copy link
Contributor

@Driedas Driedas commented Aug 6, 2019

This is my first iteration, seems to work, however am missing some automated tests that at least verify the happy path works.
Do you have any comments/suggestions on stuff to improve/change?

@redplane
Copy link
Owner

redplane commented Aug 9, 2019

I'll check and merge this PR at weekend if there is no issue.
Thanks for your CR

@Driedas
Copy link
Contributor Author

Driedas commented Sep 5, 2019

hey, is there a problem with the PR? Anything that needs to be changed, etc... ?

@redplane
Copy link
Owner

hey, is there a problem with the PR? Anything that needs to be changed, etc... ?

Sorry for my late reply. For now I just completed the pull request 6. Now I'm taking a look at your PR.
Any question I'll let you know on this.

Once again, thanks for your support :)

@redplane
Copy link
Owner

@Driedas , could you provide me Postman request & controller implementation demo ? I'll check whether it conflicts with the new merged code or not.

@Driedas
Copy link
Contributor Author

Driedas commented Sep 24, 2019

Hey, I'll implement a sample controller, but is Fiddler OK for the sample request?

@redplane
Copy link
Owner

@Driedas , yes, fiddler is ok.

@redplane
Copy link
Owner

@Driedas , any fiddler example please ?
I'm creating a new branch to merge your commit, but need an example to test your solution

@Driedas
Copy link
Contributor Author

Driedas commented Oct 28, 2019

I've tested with this one, just change the extension to .har and import into fiddler.
streamed.zip

@Driedas
Copy link
Contributor Author

Driedas commented Nov 26, 2019

hey, did you get a chance to look at this? Sorry for pushing, but have one more improvement based on the use of this package in production (support for enums in message contracts)

@redplane
Copy link
Owner

redplane commented Dec 2, 2019

Sorry for my late reply (new job, more works :D )
Base on the har file you sent me previously, I created the same request to basic-upload-streamed. This is what I got:

image

However, when I kept the old source code in MultipartFormDataFormatter, instead using the updated one:

 // set null if no content was submitted to have support for [Required]
                        if (httpContent.Headers.ContentLength.GetValueOrDefault() > 0)
                            file = new HttpFile(
                                httpContent.Headers.ContentDisposition.FileName.Trim('"'),
                                httpContent.Headers.ContentType.MediaType,
                                await httpContent.ReadAsByteArrayAsync()
                            );
                        else
                            file = null;

And changed StreamedHttpFile with HttpFile, this is what I got:

image

Could you mind telling me any advantages or differences between StreamedHttpFile and HttpFile ? :)

@Driedas
Copy link
Contributor Author

Driedas commented Dec 3, 2019

hey, no problem, thanks for sticking with it :)

The salient bits that I've added are these:
1e665ca#diff-753f377dba1d1b82a497b38be230c584R134-R147

The advantages in using the StreamedHttpFile is that at no point in time the entire attachment has to be in memory, whereas when using HttpFile it does. This does not make a terrible difference with small attachments that will all fit onto the "regular" heap in memory, Objects (and hence attachments that yield byte arrays) above ~85k in size are stored in memory on the large object heap that leads to excessive memory fragmentation which in a server environment cannot easily be reclaimed (more so with objects of varying sizes that are not multiples of each other, https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/large-object-heap). When streaming the attachment, the buffer size for the individual chunks can be configured based on whatever performs best on the server.

Below is the difference in the allocated memory on the first line of code in the respective API action methods, when trying to upload a 4MB attachment, ~6MB increase in the memory footprint for the nonstreaming endpoint as opposed to the streaming endpoint (used as baseline on the image)
image

@redplane
Copy link
Owner

redplane commented Dec 4, 2019

Thank you for your info. I'll do some tests with large file and measure IIS server for further information.
I have plan to move this library to support .Net Core also.
Any updates I'll let you know :D

@redplane
Copy link
Owner

redplane commented Jan 7, 2020

FYI, @Driedas

Your pull request has been merged in rc-3.0.0. Testing is being done before releasing a new version.

@redplane redplane merged commit 1e665ca into redplane:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants