Added background download creation. #890

Romasz · 2017-02-07T06:13:46Z

#883 - I've added two methods creating background download - one in OneDriveStorageFile and one in OneDriveService. I've also added cancellation in method responsible for getting items.

@EricVernie The GetItemsAsync(...) method of OneDriveStorageFolder allows to download up to 1000 items, due to SDK limit, otherwise it will throw exception. Another problem is that the request doesn't accept Skip(). In that case if you start with index 2 and want to download 1000 items - it will also throw exception. Do you have any ideas for that?

EricVernie · 2017-02-07T18:43:02Z

@Romasz What I understand, $skip query parameter is not supported (yet) as a valid parameter according to the documentation
https://dev.onedrive.com/odata/optional-query-parameters.htm
And when you try to use it either with Onedrive.NET SDK or directly with the restfull API you get this error
"The query specified in the Uri is not valid.
Query option 'Skip is not allowed.
To allow it set the AllowedQueryOptions property on EnableQueryAttribute
or QueryValidationsettings.
For me, but maybe i'm wrong, the problem is more from the Backend Api than the client side

btw don't hesitate to vote here https://onedrive.uservoice.com/forums/262982-onedrive/suggestions/11357844-please-add-the-odata-skip-query-option-to-a

Romasz · 2017-02-07T18:59:17Z

@EricVernie You are probably right. Then maybe GetItems(..) methods should have some more description and checkout for 1000?

You also have my votes.

ScottIsAFool · 2017-02-11T09:39:55Z

Microsoft.Toolkit.Uwp.Services/Services/OneDrive/OneDriveStorageFile.cs

+            {
+                downloader.SetRequestHeader(item.Key, item.Value.First());
+            }
+


Is there a reason this is in a Task.Run()?

@ScottIsAFool The MSDN says that if you create large number of requests then performance of UI can get degraded and we don't know if users won't use this method in a loop.

Creating a large number of transfers on the main UI thread with CreateDownload can result in degraded performance of your app's UI. If you are queuing up a large number of transfers, it is recommended that you call CreateDownload on a background worker thread as in the following example.

@Romasz please set ConfigureAwait for each task to false unless contest needs to be maintained like with BitmapImage.

Check other services / CacheBase

https://msdn.microsoft.com/en-us/library/system.threading.tasks.task.configureawait%28v=vs.110%29.aspx

You shouldn't need to worry about how and on what thread lib code usually works

@hermitdave I concur, will correct that later once I get back to project. BTW - as I understand, in this particular case where Task.Run is the last method without code to follow, putting ConfigureAwait won't change anything - am I right?

@Romasz consider using
CreateDownloadAsync instead

@hermitdave CreateDownlaodAsync needs requestBodyStream which cannot be null, therefore I'm using non-async version. I will put ConfigureAwait(false) in that case.

No its okay. I don't think Task.Run will help there. We can look at it if there is a perf issue.

ConfigureAwait might help here
var requestMessage = Provider.Drive.Items[oneDriveId].Content.Request().GetHttpRequestMessage();

await Provider.AuthenticationProvider.AuthenticateRequestAsync(requestMessage).AsAsyncAction().AsTask(cancellationToken);

@hermitdave That ones - yeah. I'm just thinking now if not to put almost whole method, starting from requestMessage under Task.Run - then there also won't be need for configuring await - what do you think about this?

Possibly.. Let me review the code later

…into dev

Romasz · 2017-02-12T07:34:38Z

Microsoft.Toolkit.Uwp.Services/Services/OneDrive/OneDriveService.cs

-            }
-
-            return await Task.Run(() => downloader.CreateDownload(requestMessage.RequestUri, destinationFile), cancellationToken);
+            return await Task.Run(


@hermitdave I've moved the wohole code to Task.Run - what do you think about it? Do we need also configuring await here?

hermitdave · 2017-02-12T08:08:42Z

btw no background dl for storage folder @Romasz ?

Romasz · 2017-02-12T08:14:29Z

@hermitdave This is harder to handle as there are many cases to be considered. Do you know if there is a method calling OneDrive to download folder as a compressed zip file?

In very simple version the method can just list every file in every subfolder and create a separate background download, but I'm not sure if it's a good idea, as there is probably a limit of background downloads being run. What do you think?

hermitdave · 2017-02-12T09:02:56Z

@Romasz we can provide user specified parallel downloads. The background downloader has a property.

@EricVernie and @deltakosh any thoughts?

Romasz · 2017-02-12T11:06:58Z

@hermitdave You mean the TransferGroup property? I will need to give this couple of tries in upcoming days.

hermitdave · 2017-02-12T11:33:13Z

I meant https://docs.microsoft.com/en-us/uwp/api/windows.networking.backgroundtransfer.backgrounddownloader#Windows_Networking_BackgroundTransfer_BackgroundDownloader_GetCurrentDownloadsForTransferGroupAsync_Windows_Networking_BackgroundTransfer_BackgroundTransferGroup_
Which can be called to get list of items and thereby the count.

Romasz · 2017-02-12T11:51:51Z

@hermitdave I will see what I can do, though need some time for this (quite lot of work around).

hermitdave · 2017-02-12T13:37:28Z

@Romasz true that can be added later

pedrolamas · 2017-02-14T15:41:06Z

Microsoft.Toolkit.Uwp.Services/Services/OneDrive/OneDriveService.cs

+                async () =>
+                {
+                    var requestMessage = Provider.Drive.Items[oneDriveId].Content.Request().GetHttpRequestMessage();
+                    await Provider.AuthenticationProvider.AuthenticateRequestAsync(requestMessage).AsAsyncAction().AsTask(cancellationToken);


Using .AsAsyncAction().AsTask() just to pass a CancellationToken will probably do nothing for this method.

it's the AuthenticateRequestAsync that should have CancellationToken support!

@pedrolamas I wonder if we need those cancellation tokens there, as this part of code runs on separate thread which has it's own cancellation token Task.Run(Action, cancellationToken).

I would like to know how this behaves in case od cancel signal, the thread is ended instantly/methods interrupted? Though I don't know where I can find such information and don't have much time now to go across reference source.

To be fair, I'm not quite fond with using Task.Run there, but I can understand your reasoning!

Libraries should not lie about async work, and there is a "rule" about not using Task.Run() inside of them and just let the developers do that from the UI!

http://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-even-in.html

the problem is that in this instance the docs are clear that executing this from UI thread will result in perf issues.

@pedrolamas I think we have two choices - leave as it is now, even against etiquette or provide a description in method like 'Consider running this method on separate thread while creating large number of download operations'.

Personally I like the first solution, but I've nothing against the second and I completely understand your reasons.

Etiquette is great but we must be pragmatic. We have to do everything that we can to simplify usages of the API. If we know that this method has to run on a thread, then run it on a thread.

Fair enough, in that case I think this is ready to merge!

Added background download creation.

405826a

dnfclas added the cla-already-signed label Feb 7, 2017

ScottIsAFool reviewed Feb 11, 2017

View reviewed changes

Romasz added 2 commits February 12, 2017 08:30

Changed Task.Run in create bg download

cbad44c

Merge branch 'dev' of https://github.com/Microsoft/UWPCommunityToolkit …

f4682be

…into dev

Romasz commented Feb 12, 2017

View reviewed changes

hermitdave approved these changes Feb 12, 2017

View reviewed changes

deltakosh approved these changes Feb 13, 2017

View reviewed changes

pedrolamas reviewed Feb 14, 2017

View reviewed changes

pedrolamas approved these changes Feb 14, 2017

View reviewed changes

deltakosh merged commit 6e7b94c into CommunityToolkit:dev Feb 14, 2017

Added background download creation. #890

Added background download creation. #890

Uh oh!

Conversation

Romasz commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricVernie commented Feb 7, 2017

Uh oh!

Romasz commented Feb 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermitdave Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermitdave commented Feb 12, 2017

Uh oh!

Romasz commented Feb 12, 2017

Uh oh!

hermitdave commented Feb 12, 2017

Uh oh!

Romasz commented Feb 12, 2017

Uh oh!

hermitdave commented Feb 12, 2017

Uh oh!

Romasz commented Feb 12, 2017

Uh oh!

hermitdave commented Feb 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deltakosh Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Romasz commented Feb 7, 2017 •

edited

Loading

hermitdave Feb 11, 2017 •

edited

Loading

deltakosh Feb 14, 2017 •

edited

Loading