Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .spelling
Original file line number Diff line number Diff line change
Expand Up @@ -1104,6 +1104,8 @@ v6.0.
#region test/tools/WebListener/README.md Overrides
- test/tools/WebListener/README.md
Auth
NoResume
NTLM
NumberBytes
ResponseHeaders
#endregion
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Contributor Author

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.


#endregion

#endregion Virtual Properties
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe private?
This is only used once - we could remove this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();

Expand All @@ -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))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wont change anything in the web cmdlets...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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;
}

Expand Down Expand Up @@ -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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. I'll add it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestions for wording?

The remote server indicates the file is already complete. OutFile will not be written to.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

The OutFile "{0}" is the same size as the remote file. The file will not be re-downloaded.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better. fixed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file name can be long. I suggest move it to end:
The file will not be re-downloaded because the remote file is the same size as the OutFile: {0}.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,20 @@ internal static void WriteToStream(byte[] input, Stream output)
/// <param name="cmdlet"></param>
internal static void SaveStreamToFile(Stream stream, string filePath, PSCmdlet cmdlet)
{
using (FileStream output = File.Create(filePath))
// If the web cmdlet should resume, append the file instead of overwriting.
if(cmdlet is WebRequestPSCmdlet webCmdlet && webCmdlet.ShouldResume)
{
WriteToStream(stream, output, cmdlet);
using (FileStream output = new FileStream(filePath, FileMode.Append, FileAccess.Write, FileShare.Read))
{
WriteToStream(stream, output, cmdlet);
}
}
else
{
using (FileStream output = File.Create(filePath))
{
WriteToStream(stream, output, cmdlet);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,10 @@
<value>Path '{0}' is not a file system path. Please specify the path to a file in the file system.</value>
</data>
<data name="OutFileMissing" xml:space="preserve">
<value>The cmdlet cannot run because the following parameter is missing: OutFile. Provide a valid OutFile parameter value when using the PassThru parameter, then retry.</value>
<value>The cmdlet cannot run because the following parameter is missing: OutFile. Provide a valid OutFile parameter value when using the {0} parameter, then retry.</value>
</data>
<data name="OutFileWritingSkipped" xml:space="preserve">
<value>The file will not be re-downloaded because the remote file is the same size as the OutFile: {0}</value>
</data>
<data name="ProxyCredentialConflict" xml:space="preserve">
<value>The cmdlet cannot run because the following conflicting parameters are specified: ProxyCredential and ProxyUseDefaultCredentials. Specify either ProxyCredential or ProxyUseDefaultCredentials, then retry.</value>
Expand Down Expand Up @@ -249,6 +252,9 @@
<data name="WebMethodInvocationVerboseMsg" xml:space="preserve">
<value>{0} {1} with {2}-byte payload</value>
</data>
<data name="WebMethodResumeFailedVerboseMsg" xml:space="preserve">
<value>The remote server indicated it could not resume downloading. The local file will be overwritten.</value>
</data>
<data name="WebResponseVerboseMsg" xml:space="preserve">
<value>received {0}-byte response of content type {1}</value>
</data>
Expand Down
Loading