Repository navigation
Invoke-WebRequest/RestMethod: Rename TimeoutSec to ConnectTimeoutSec and alias to TimeoutSec and add NetworkTimeoutSec timeout #19519
Description
Rename the parameter with an alias based on this wg-powershell-cmdlets review comment
@PowerShell/wg-powershell-cmdlets reviewed this. Based on the documentation currently, -TimeoutSec is for the initial connection and the definition of this parameter should not change. We suggest renaming this parameter to -ConnectTimeoutSec and alias to -TimeoutSec. For this new capability, we propose adding a new -NetworkTimeoutSec (default to 0 meaning infinite) to handle the case of intermittent network issues.
Originally posted by Steve Lee (@SteveL-MSFT) in #18005 (comment)
Activity
[-]Rename TimeoutSec to ConnectTimeoutSec and alias to TimeoutSec and add NetworkTimeoutSec timeout[/-][+]Invoke-WebRequest/RestMethod: Rename TimeoutSec to ConnectTimeoutSec and alias to TimeoutSec and add NetworkTimeoutSec timeout[/+]kasini3000 commentedon Apr 16, 2023
how many timeout with http client？It's best to add it all at once.
connectTimeout
sendTimeout
readTimeout
https://learn.microsoft.com/en-us/dotnet/api/system.net.httplistenertimeoutmanager?view=net-7.0
I saw you adding a cancellation token to IWR. Do you have time to address this issue?
PowerShell/ThreadJob#26
stevenebutler commentedon Apr 16, 2023
iSazonov commentedon Apr 17, 2023
We have to proceed from realistic scenarios. There are actually two types (1) get/send small data with a request, (2) upload/download file.
From this it is easy to draw the right conclusion how
Timeoutshould work.stevenebutler commentedon Apr 17, 2023
You could also be uploading/downloading data you have no intention of storing in a file so I'm not sure it's as clear cut as having two options and an easy, automatic choice between what to do with the parameter.
Simplest implementation is a straight timeout on the receipt of all data but as has been discussed before this doesn't work well when we don't know ahead of time how long the download will take because of varying loads/network speeds etc. The main advantage of this sort of timeout is it could be implemented by using
cts.CancelAfter(timeout)before reading the response stream.Another option is a "no packet received" timeout which would reset every time some data is received. This is more challenging to implement because there's no direct support for this kind of timeout in the core libraries used.
Unfortunately, the WG didn't really give much guidance when writing that statement as to whether it was intending it to be a timeout for the overall download time or a "no packet received in x period" timeout or some hybrid of the two.
Both might be useful depending on the use case but it may be surprising if the same parameter has a different behaviour depending on whether we are downloading to a file or not.
Ilya (@iSazonov) could you be a bit more specific about what you expect since others may have a different view point and it's good to be explicit.
iSazonov commentedon Apr 17, 2023
#18005 (comment)
In short, the only meaning of this parameter is the timeout for any atomic operation.
(In any case cmdlet will return a generic error and it does not matter what the reason is - DNS, connection, network timeout or any other of an infinite number of reasons. Although actually this reason will be an InnerException.)
kasini3000 commentedon Apr 17, 2023
Rename TimeoutSec to ConnectTimeoutSec
add NoDataReceiveTimeoutSec
sounds good
stevenebutler commentedon Apr 19, 2023
Ilya (@iSazonov) - so what you're saying is the timeout should only ever mean "length of time between received data chunks/packets/whatever" has been exceeded, and should in no way apply to the entire length of the download, regardless of the use case involved.
There might be some benefit in having an additional parameter that caps the overall download time, but that is probably overkill and wasn't requested by the WG. Do you have any thoughts on that?
iSazonov commentedon Apr 19, 2023
stevenebutler The parameter you ask makes no sense because download time is unpredictable. Any component (client, server, network) can be either faster or slower. Moreover, the size of the file is in general also unpredictable. (For example, the size of a daily log file can be 1 Mb or 10Gb.)
You can say that "my file always uploads no more than a minute and I agree to wait even an hour". But a timeout of one hour makes no sense because the server will close the session if there is a delay of any operation for about a minute, and a high-loaded server will do it even earlier.
CarloToso commentedon Apr 20, 2023
I think the
-NetworkTimeoutSecparameter is meant to handle this issue #16122 (we could also avoid the new parameter and set a timeout at 5 minutes with no data recieved)stevenebutler commentedon Apr 20, 2023
From the WG notes they suggested defaulting to not have any timeout which is leaving the default situation where it will hang until someone CTRL-C the download if the OS doesn't report the loss of connection. Are you suggesting the default should be something other than no timeout?
If the WG have decided on a course of action, is that what should be implemented, or is it still up for further debate?
CarloToso commentedon Apr 20, 2023
I don't know if it's up for further debate, but in my opinion we should follow PS5 behaviour described in #12249. Would you be open to reconsider Steve Lee (@SteveL-MSFT)
stevenebutler commentedon Apr 23, 2023
PR above adds the feature as per the discussion here, and with the timeout defaulting to disabled. If WG wants to change the default that will be trivial to do.