microsoft / WindowsDevicePortalWrapper

A client library that wraps the Windows Device Portal REST APIs.
MIT License
182 stars 87 forks source link

AppX deployment fails for really large packages #141

Open WilliamsJason opened 8 years ago

WilliamsJason commented 8 years ago

Looks like MemoryStream isn't large enough. I tried swapping to a FileStream backed by a temporary file and that also failed, but that may be a server issue. We'll need to get that investigated/fixed before it's worth addressing the MemoryStream fix (FileStream will probably be slower and use up more storage space but may be the easiest solution).

WilliamsJason commented 8 years ago

Just to add a couple more details--MemoryStream fails with an exception about 'stream too long' while the FileStream got an OperationCanceledException much further along in the process.

david-c-kline commented 8 years ago

can you define "really large"? :)

hpsin commented 8 years ago

4GB + aka video games. You can guess why with a number like that (currently we haven't determined why, but it's pretty auspicious). Note that this may be an issue with Device Portal itself (TBD) and also an issue in browsers.

On Aug 10, 2016 5:07 PM, "David Kline" notifications@github.com wrote:

can you define "really large"? :)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Microsoft/WindowsDevicePortalWrapper/issues/141#issuecomment-239041870, or mute the thread https://github.com/notifications/unsubscribe-auth/ABltO082SDrSrWHzP8TRkkNdQUPZl5Sfks5qemezgaJpZM4Jgljx .

WilliamsJason commented 8 years ago

4 GB is probably the magic number for WDP. WDPW will actually choke on a smaller size due to the MemoryStream implementation (possibly 256 MB, maybe as high as 2 GB if resources are plentiful, see this stack overflow thread for more details).

WilliamsJason commented 7 years ago

Just an update here--The underlying WDP issues should all be resolved shortly but the MemoryStream implementation in the wrapper project still needs to be updated.

dotMorten commented 7 years ago

Suggestion: Instead of loading all those files into memory, post an HttpMultipartContent that consists of HttpStreamContent items. It would GREATLY reduce the code in InstallApplicationAsync, and it would also reduce memory consumption to the size of the buffer that's used to stream from the files to the network (which would only be a few kb). All that code with the boundaries etc are completely overkill, since that is already supported by the HttpContent classes: https://github.com/Microsoft/WindowsDevicePortalWrapper/blob/master/WindowsDevicePortalWrapper/WindowsDevicePortalWrapper.Shared/Core/AppDeployment.cs#L122%2DL170 Could be reduced to

var content = new HttpMultipartContent();
foreach (string dependencyFile in dependencyFileNames)
    content.Add(new HttpStreamContent(File.OpenRead(dependencyFile).AsInputStream()));
WilliamsJason commented 7 years ago

This sounds like exactly the solution I was looking for, thank you! I suspect this will be MUCH more performant as well which I was also concerned about.

I'm not sure when I'll get time to make this change, if anyone else wants to make it feel free and I'll review the PR. Otherwise I'll get to it when I am able.

david-c-kline commented 7 years ago

There was an issue (need to investigate to see if it is still there) with how the Device Portal parsed multipart content that required the code currently in the wrapper.

If I can get some time, I'll check if the issue still exists.

david-c-kline commented 7 years ago

Closed by mistake... sorry

dotMorten commented 7 years ago

I'm guessing it's because of this bug: https://insider.windows.com/FeedbackHub/fb?contextid=519&feedbackid=19a5af49-38f4-409a-b464-e66f80679545&form=1

We could work around that by using a custom HttpContent object and write to the output stream file by file in a buffered manner. I can take a stab a that.

dotMorten commented 7 years ago

Took a first stab at it (see PR linked above). It would need some serious testing

WilliamsJason commented 7 years ago

Awesome! I have the setup to test on Xbox but probably won't have the time until next week. I'll try to carve out a block of time then. It's possible others will have time to try other device types, but the UWP/NET difference is probably more likely to yield issues than the different device types (it's easier for me to test the .NET so if someone else does have time, focus on UWP first if they are otherwise equal for you).

david-c-kline commented 7 years ago

I should have some cycles to try this on a HoloLens later this week.

Thank you for taking this (and the await change) on! David

dotMorten commented 7 years ago

Fixed some bugs tonight, so you should have a lot more luck in your testing

dotMorten commented 7 years ago

@hpsin Could you try your 4Gb package with this PR and verify memory consumption is better? If not, we might have to explicitly chunk the buffering (simple change), but AFAIK it does that by default.

hpsin commented 7 years ago

I tried a 3.5 GB file and it failed a minute or two in during copying, with "A task was canceled". The same appx without a 3.4 GB VHD inside of it installs fine. Xbox is seeing the same behavior. Unsure what is causing this.

dotMorten commented 7 years ago

@hpsin Does it work without my PR? (provided you have the memory available to pull it off - ie run x64!)

david-c-kline commented 6 years ago

This MIGHT be app architecture related. I have found, recently, that using a browser to try and copy a large (>3GB package) failed if the browser was a 32bit app. Switching to a 64 bit browser allowed me to copy the package.