-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Add -Resume Feature to Web Cmdlets #6447
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -354,6 +354,12 @@ public virtual string CustomMethod | |
| [Parameter] | ||
| public virtual SwitchParameter PassThru { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Resumes downloading a partial or incomplete file. OutFile is required. | ||
| /// </summary> | ||
| [Parameter] | ||
| public virtual SwitchParameter Resume { get; set; } | ||
|
|
||
| #endregion | ||
|
|
||
| #endregion Virtual Properties | ||
|
|
@@ -512,7 +518,15 @@ internal virtual void ValidateParameters() | |
| if (PassThru && (OutFile == null)) | ||
| { | ||
| ErrorRecord error = GetValidationError(WebCmdletStrings.OutFileMissing, | ||
| "WebCmdletOutFileMissingException"); | ||
| "WebCmdletOutFileMissingException", nameof(PassThru)); | ||
| ThrowTerminatingError(error); | ||
| } | ||
|
|
||
| // Resume requires OutFile. | ||
| if (Resume.IsPresent && OutFile == null) | ||
| { | ||
| ErrorRecord error = GetValidationError(WebCmdletStrings.OutFileMissing, | ||
| "WebCmdletOutFileMissingException", nameof(Resume)); | ||
| ThrowTerminatingError(error); | ||
| } | ||
| } | ||
|
|
@@ -637,6 +651,14 @@ internal bool ShouldWriteToPipeline | |
| get { return (!ShouldSaveToOutFile || PassThru); } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Determines whether writing to a file should Resume and append rather than overwrite. | ||
| /// </summary> | ||
| internal bool ShouldResume | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe private?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs to be internal. It is accessed by another class in the same assembly. Private would not make it available. |
||
| { | ||
| get { return (Resume.IsPresent && _resumeSuccess); } | ||
| } | ||
|
|
||
| #endregion Helper Properties | ||
|
|
||
| #region Helper Methods | ||
|
|
@@ -860,6 +882,16 @@ public abstract partial class WebRequestPSCmdlet : PSCmdlet | |
| /// </summary> | ||
| internal int _maximumFollowRelLink = Int32.MaxValue; | ||
|
|
||
| /// <summary> | ||
| /// The remote endpoint returned a 206 status code indicating successful resume. | ||
| /// </summary> | ||
| private bool _resumeSuccess = false; | ||
|
|
||
| /// <summary> | ||
| /// The current size of the local file being resumed. | ||
| /// </summary> | ||
| private long _resumeFileSize = 0; | ||
|
|
||
| private HttpMethod GetHttpMethod(WebRequestMethod method) | ||
| { | ||
| switch (Method) | ||
|
|
@@ -1062,6 +1094,22 @@ internal virtual HttpRequestMessage GetRequest(Uri uri, bool stripAuthorization) | |
| } | ||
| } | ||
|
|
||
| // If the file to resume downloading exists, create the Range request header using the file size. | ||
| // If not, create a Range to request the entire file. | ||
| if (Resume.IsPresent) | ||
| { | ||
| var fileInfo = new FileInfo(QualifiedOutFile); | ||
| if (fileInfo.Exists) | ||
| { | ||
| request.Headers.Range = new RangeHeaderValue(fileInfo.Length, null); | ||
| _resumeFileSize = fileInfo.Length; | ||
| } | ||
| else | ||
| { | ||
| request.Headers.Range = new RangeHeaderValue(0, null); | ||
| } | ||
| } | ||
|
|
||
| // Some web sites (e.g. Twitter) will return exception on POST when Expect100 is sent | ||
| // Default behavior is continue to send body content anyway after a short period | ||
| // Here it send the two part as a whole. | ||
|
|
@@ -1233,6 +1281,9 @@ internal virtual HttpResponseMessage GetResponse(HttpClient client, HttpRequestM | |
| if (client == null) { throw new ArgumentNullException("client"); } | ||
| if (request == null) { throw new ArgumentNullException("request"); } | ||
|
|
||
| // Track the current URI being used by various requests and re-requests. | ||
| var currentUri = request.RequestUri; | ||
|
|
||
| _cancelToken = new CancellationTokenSource(); | ||
| HttpResponseMessage response = client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, _cancelToken.Token).GetAwaiter().GetResult(); | ||
|
|
||
|
|
@@ -1256,14 +1307,51 @@ internal virtual HttpResponseMessage GetResponse(HttpClient client, HttpRequestM | |
| } | ||
|
|
||
| // recreate the HttpClient with redirection enabled since the first call suppressed redirection | ||
| currentUri = new Uri(request.RequestUri, response.Headers.Location); | ||
| using (client = GetHttpClient(false)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have plans to move to .Net Core 2.1 in days - in Preview2 we'll get new HttpClient. I suppose we should freeze this change up to this point.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That wont change anything in the web cmdlets...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll clarify: The public API's for HttpClient are not changing (as far as I'm aware) and the new HttpClientFactory is a feature that would require more refactoring to implement than could be accomplished before RC and GA for 6.1.0. They would also require RFC as they would incorporate new features that have more than a few design considerations to address. There should be no significant changes that impact code points in the Web Cmdltes. But we should get a bit of a performance boost. |
||
| using (HttpRequestMessage redirectRequest = GetRequest(new Uri(request.RequestUri, response.Headers.Location), stripAuthorization:true)) | ||
| using (HttpRequestMessage redirectRequest = GetRequest(currentUri, stripAuthorization:true)) | ||
| { | ||
| FillRequestStream(redirectRequest); | ||
| _cancelToken = new CancellationTokenSource(); | ||
| response = client.SendAsync(redirectRequest, HttpCompletionOption.ResponseHeadersRead, _cancelToken.Token).GetAwaiter().GetResult(); | ||
| } | ||
| } | ||
|
|
||
| // Request again without the Range header because the server indicated the range was not satisfiable. | ||
| // This happens when the local file is larger than the remote file. | ||
| // If the size of the remote file is the same as the local file, there is nothing to resume. | ||
| if (Resume.IsPresent && | ||
| response.StatusCode == HttpStatusCode.RequestedRangeNotSatisfiable && | ||
| (response.Content.Headers.ContentRange.HasLength && | ||
| response.Content.Headers.ContentRange.Length != _resumeFileSize)) | ||
| { | ||
| _cancelToken.Cancel(); | ||
|
|
||
| WriteVerbose(WebCmdletStrings.WebMethodResumeFailedVerboseMsg); | ||
|
|
||
| // Disable the Resume switch so the subsequent calls to GetResponse() and FillRequestStream() | ||
| // are treated as a standard -OutFile request. This also disables appending local file. | ||
| Resume = new SwitchParameter(false); | ||
|
|
||
| using (HttpRequestMessage requestWithoutRange = GetRequest(currentUri, stripAuthorization:false)) | ||
| { | ||
| FillRequestStream(requestWithoutRange); | ||
| long requestContentLength = 0; | ||
| if (requestWithoutRange.Content != null) | ||
| requestContentLength = requestWithoutRange.Content.Headers.ContentLength.Value; | ||
|
|
||
| string reqVerboseMsg = String.Format(CultureInfo.CurrentCulture, | ||
| WebCmdletStrings.WebMethodInvocationVerboseMsg, | ||
| requestWithoutRange.Method, | ||
| requestWithoutRange.RequestUri, | ||
| requestContentLength); | ||
| WriteVerbose(reqVerboseMsg); | ||
|
|
||
| return GetResponse(client, requestWithoutRange, stripAuthorization); | ||
| } | ||
| } | ||
|
|
||
| _resumeSuccess = response.StatusCode == HttpStatusCode.PartialContent; | ||
| return response; | ||
| } | ||
|
|
||
|
|
@@ -1336,7 +1424,22 @@ protected override void ProcessRecord() | |
| contentType); | ||
| WriteVerbose(respVerboseMsg); | ||
|
|
||
| if (!response.IsSuccessStatusCode) | ||
| bool _isSuccess = response.IsSuccessStatusCode; | ||
|
|
||
| // Check if the Resume range was not satisfiable because the file already completed downloading. | ||
| // This happens when the local file is the same size as the remote file. | ||
| if (Resume.IsPresent && | ||
| response.StatusCode == HttpStatusCode.RequestedRangeNotSatisfiable && | ||
| response.Content.Headers.ContentRange.HasLength && | ||
| response.Content.Headers.ContentRange.Length == _resumeFileSize) | ||
| { | ||
| _isSuccess = true; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have a Verbose message informing the user nothing was downloaded? Edge case where the local file is same size as remote file, but the files are different.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea. I'll add it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestions for wording?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Much better. fixed
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The file name can be long. I suggest move it to end:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed. fixed |
||
| WriteVerbose(String.Format(CultureInfo.CurrentCulture, WebCmdletStrings.OutFileWritingSkipped, OutFile)); | ||
| // Disable writing to the OutFile. | ||
| OutFile = null; | ||
| } | ||
|
|
||
| if (!_isSuccess) | ||
| { | ||
| string message = String.Format(CultureInfo.CurrentCulture, WebCmdletStrings.ResponseStatusCodeFailure, | ||
| (int)response.StatusCode, response.ReasonPhrase); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we still use
virtual.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a base class. In theory, maybe we add a new cmdlets that needs to overwrite this.. or not have it as a parameter.