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.

[davidsh]

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.

I agree we should throw PNSE instead of NIE if we are throwing exceptions.

The question we run into is whether or not we should throw exceptions which can break apps that don't expect the exception. Being silent (noop) might then lead to getting bugs raised since devs will think it should work. This problem of deciding what to do impacts several problems we have in HttpClientHandler that can't work on UWP, for example.

So, it would be good to get a general design pattern in place for how to handle this platform differences.

cc: @weshaggard @karelz @CIPop

[ViktorHofer]

That's really tough. @stephentoub as you explained thoroughly I think we can't fully satisfy the HttpWebRequest API contract with our current implementation. I would be fine making it a NOP but not 100% happy...

[RickStrahl]

An attribute marker that shows a warning at compile time for this sort of thing would be useful so you can catch issues like this before they bite you at runtime especially with code already in production and getting ported.

[Obsolete] based on platform would work but something more specific might be really useful for many other use cases.

[karelz]

@RickStrahl we have tooling for that: https://github.com/dotnet/corefx/wiki/ApiCompat

[ViktorHofer]

Maybe it's also worth to mention that with our current implementation we are also facing the issue that we are creating a dedicated HttpClient instance for each request...

[karelz]

And HttpWebRequest is already called out here: https://github.com/terrajobst/platform-compat/blob/master/docs/DE0003.md as legacy API

[RickStrahl]

Not sure how that works - doesn't HttpClient on full framework use HttpWebRequest as its implementation? Has that changed?

[davidsh]

doesn't HttpClient on full framework use HttpWebRequest as its implementation?

Yes. But that architecture is not how it works on .NET Core.

[danmoseley]

@karelz @davidsh just to be explicit, @ViktorHofer can't look at this as he's engaged on a DCR this week.

[karelz]

@davidsh can you please help with this fix?
cc @DavidGoll

[karelz]

Triage: We agree that both properties should do "noop". Currently we throw for both. Code change is needed.

We should also document it in https://github.com/dotnet/corefx/wiki/ApiCompat (can happen during Escrow)

[loop-evgeny]

being silent (noop) might then lead to getting bugs raised since devs will think it should work.

Yep, this dev thought it should work and raised a bug: https://github.com/dotnet/corefx/issues/34550

[coderb]

@stephentoub "If we feel we need to actually timeout".

anyone writing production code that reads from a socket needs to "actually timeout" if they want their code to be reliable! please solve this! the easiest lowest overhead solution is to set the timeout on the socket (ie via ioctl() to the underlying o/s). this is supported on all platforms and is generally efficient! pretty please!!!

[stephentoub]

the easiest lowest overhead solution is to set the timeout on the socket (ie via ioctl() to the underlying o/s). this is supported on all platforms

The timeouts on sockets on such platforms are for synchronous reads/writes, not asynchronous.

anyone writing production code that reads from a socket needs to "actually timeout" if they want their code to be reliable!

Cancellation is implemented all the way through HttpClient, and cancellation can be triggered for whatever reason, including in response to a timeout. The comment here was about a very specific kind of timeout, that of individual read and write operations, with each operation getting its own timeout. That is also possible today, with each Read/WriteAsync operation accepting a CancellationToken, enabling cancellation of the individual operation; it just doesn't have a simple built-in set-a-timeout-once-and-it'll-be-applied-a-new-to-each-operation mode.

Regardless, this issue has been closed for several years, and most folks do not monitor closed issues. If you have some functionality that you would like to see supported, please either find an open issue for the functionality and comment on that, or open a new one.

Thanks.

[coderb]

@stephentoub there is 15+ years worth of netfx code that has been written much of which uses synchronous apis for networking. having read/write timeouts on synchronous socket operations be no-ops is a huge land mine in that it will lead to difficult to reproduce problems for devs.

now that netcore is advertised "on par" with the features of netfx and the future of dotnet development there is going to be more "upgrading" of netfx applications to netcore and this latent issue is going to cause lots of headaches.

i guess i'll try to port my codebase to async but it's going to be a ton of work. i've got 200+ projects many of which are network oriented.

---

edit: an additional point. you've chosen to include synchronous networking apis in dotnet core. i would suggest that doing so strongly implies that read/write timeouts need work or else the apis are effectively broken. you can't really write a reliable network app without timing out sockets. it's usually a rare condition but hanging connections due to lost RST/FIN is a scenario that needs to be accounted for in code that is robust and not just a toy.