HttpWebRequest.ReadWriteTimeout property returns NotImplemented

[RickStrahl]

Looks like HttpWebRequest.ReadWriteTimeout throws a NotImplemented exception.
Also HttpWebRequest.ConnectionGroupName

Expected:
Either work with expected behavior, or remove from NET Core altogether as this will result in an unexpected runtime error.

[davidsh]

There is no ReadWriteTimeout on The System.Net.WebRequest class.

Can you provide a link to the source code in question in .NET Core?

[davidsh]

Perhaps you meant the "Timeout" property in WebRequest.
https://github.com/dotnet/corefx/blob/master/src/System.Net.Requests/src/System/Net/WebRequest.cs#L469

The WebRequest class is an abstract class and many of the properties are virtual and aren't implemented in this base class.

The behavior is the same on .NET Framework:
https://github.com/Microsoft/referencesource/blob/master/System/net/System/Net/WebRequest.cs#L794

[RickStrahl]

Sorry HttpWebRequest... updated the title to reflect.

https://docs.microsoft.com/en-us/dotnet/api/system.net.httpwebrequest.readwritetimeout?view=netframework-4.7#System_Net_HttpWebRequest_ReadWriteTimeout

Here's what I see when debugging through with .NET Core 2:

This code compiles in both .NET 4.5 and netcoreapp2.0, but gives the not implemented exception on Core and runs as expected on 45.

[davidsh]

Thanks for clarifying. Yes, I can see that in .NET Core it is currently left as NotImplemented which is different than .NET Framework.

@ViktorHofer Do you know why your prior PRs for HttpWebRequest (#19118) did not fix this?

[ViktorHofer]

Not sure. I'm definitely surprised. Will take a look later today.

[stephentoub]

It looks like HttpWebRequest.ConnectionGroupName also throws a NotImplementedException? That should probably be addressed as well, though I expect it's less impactful.

[ViktorHofer]

Do you know why your prior PRs for HttpWebRequest (#19118) did not fix this?

Sorry I had to take one day off. Yes the reason is this:
throw NotImplemented.ByDesignWithMessage(SR.net_PropertyNotImplementedException);
I supposed we aren't implementing the property because of the NotImplemented.ByDesignWithMessage call. So I guess this should be implemented now?

[ViktorHofer]

It looks like HttpWebRequest.ConnectionGroupName also throws a NotImplementedException? That should probably be addressed as well, though I expect it's less impactful.

Same reason as ReadWriteTimeout.

[ViktorHofer]

@davidsh @stephentoub We are using HttpClient in HttpWebRequest which doesn't give us access to the request stream, therefore we can't set the Read and Write properties: https://referencesource.microsoft.com/#System/net/System/Net/HttpWebRequest.cs,3343

How should we proceed? I noticed that WebRequestHandler would have the necessary properties, but that type isn't ported yet to core: https://apisof.net/catalog/System.Net.Http.WebRequestHandler.ReadWriteTimeout

[ViktorHofer]

Same applies to ConnectionGroupName. We can't use this property with the current HttpClient impl. Should we make it a NOP or keep it throwing?

[davidsh]

I noticed that WebRequestHandler would have the necessary properties, but that type isn't ported yet to core: https://apisof.net/catalog/System.Net.Http.WebRequestHandler.ReadWriteTimeout

WebRequestHandler class will never be ported to .NET Core.

[davidsh]

Should we make it a NOP or keep it throwing?

That is a good question and applies generally to properties we can't implement due to architecture differences.

[RickStrahl]

NOP is better perhaps with a conditional [Obsolete] based on platform so you can catch this at Compile time.

So is the behavior of this timeout handled by the Timeout property in Core? The only reason to use ReadWriteTimeout was that it was necessary to deal with timeouts properly in older versions.

[stephentoub]

Same applies to ConnectionGroupName

This can just be:

public override string ConnectionGroupName { get; set; }

The implementation will end up ignoring the value, but that's ok.

We are using a HttpClient in HttpWebRequest which doesn't give us access to the request stream

Well, HttpClient does, but the implementation of HttpWebRequest isn't taking advantage of that, and is instead always buffering the request stream entirely. From a ReadWriteTimeout perspective, though, that means it'll never timeout, because all requests to it would be satisfied synchronously. So for the request stream side of things, ReadWriteTimeout could simply be a roundtripping nop and it'd be fine.

The response side is harder. The right way to implement it would probably be to set the Read/WriteTimeout properties on the response stream if the response stream supports timeouts. But the response streams used by both Windows and Unix have CanTimeout==false, which means that won't actually make a difference in practice (it would "light up" and start working in the future if we ever enabled timeout support). An alternative approach would be to wrap the response stream in one that added a cancellation token to each read/write, where the token would be canceled after the specified timeout. That would probably work, but it's also a fair amount of work and would likely add non-trivial overheads on top of the already significant overheads HttpWebRequest has in core.

We should not keep the current behavior of throwing NotImplementedException. Worst case we should change it to throw PlatformNotSupportedException. But I'd also be ok in this case just making it a roundtripping nop, i.e. validate the arguments, store the data, return the stored data from the getter, but not actually respect it in terms of actually timing out. If we feel we need to actually timeout, then we either need to implement timeouts on the response streams used by HttpClient or do the response stream wrapping with cancellation token.