Http file streaming#218
Conversation
… and continue on background threads.
…onfigureAwait, and remove await where not needed
…tch to ui thread, it's easier to just await and continue on the same thread here.
…mory consumption when installing large files.
Refactored use of async. (microsoft#215)
|
I'm starting to look at this finally. For a small appx with a single dependency on Xbox it succeeded but for a larger package it's failing with the following: Cannot access a disposed object. Still digging in, but I'll try to give you info as I get it in case you know what the issue is and how to address it. |
|
There's a pretty good description of the problem in this PR: aspnet/KestrelHttpServer#750. |
|
Hmm. That's on the response though. This is failing while setting up the request still. I also noticed that sometimes it's an abort exception instead of the disposed exception. I think something is closing the out stream that we are copying the file to after a pretty good number of bytes. I'll try a few more things. |
|
I tried with a few different buffer sizes on the CopyToAsync of the large appx (up to a 1 GB buffer), but no luck. It super consistently fails at the same duration per buffer size though--slightly different duration for each buffer size but around 1.5 minutes for all of them. Could be a certain part of the appx file or count of bytes, could be something else. |
|
I tried 4Gb last night in UWP, and it uploaded fine, and memory consumption stayed around 15mb (but failed install which could have been a different reason. Didn't get a chance to dive in why yet, since the file copy over 2GHz wifi took a looooong time, and I needed to go to bed :-) |
david-c-kline
left a comment
david-c-kline
left a comment
There was a problem hiding this comment.
A change to better support installs on UWP unfortunately introduced a merge issue for this PR. Please feel free to ping me to help work through any merge issues you encounter.
David
| length += (ulong)(new FileInfo(item).Length + boundaryLength); | ||
| } | ||
| length += (ulong)(boundaryLength + 4); | ||
| return true; |
There was a problem hiding this comment.
This method should return false in error scenarios or it should be renamed ComputeLength and throw on error.
| /// </summary> | ||
| internal sealed class HttpMultipartFileContent : IHttpContent | ||
| { | ||
| private List<string> items = new List<string>(); |
There was a problem hiding this comment.
Throughout, please add comment blocks similar to the other classes in the code :)
| }); | ||
| } | ||
|
|
||
| private async Task<ulong> WriteToStreamAsyncTask(IOutputStream outputStream, Action<ulong> progress) |
There was a problem hiding this comment.
Issue #228 will necessitate reworking this for the UWP version of WDPW.
| /// <param name="requestStream">Optional stream containing data for the request body.</param> | ||
| /// <param name="requestStreamContentType">The type of that request body data.</param> | ||
| /// <returns>Task tracking the completion of the POST request</returns> | ||
| private async Task<Stream> PostAsync( |
There was a problem hiding this comment.
does removing async here create any issues with calling code?
|
I'm going to close this PR since I pulled it into my Fork and have a new PR up with the same changes. Thanks! |
6 participants
This is a first stab at using a custom http multipart content to avoid large memory consumption when installing large files.
Please DO NOT merge this yet. It needs some significant testing for the
InstallApplicationAsynccall, and I don't have all the scenarios currently available to test them all. I'm submitting this for issue #141 and asking for people to please give this a go and test it.The UWP and .NET implementations are quite different and would need separate testing.