make Socket useable after cancellation

[wfurt]

contributes to #69397

We already have logic to don't disconnect coket after sync Read. This change makes it consistent for ReadAsync.
I personally don't see reason why the cancellation (or timeout) should change status of the socket since they are really independent. And I don't see any risk that we would loose data.

I added test for both sync & async versions.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

contributes to #69397

We already have logic to don't disconnect coket after sync Read. This change makes it consistent for ReadAsync.
I personally don't see reason why the cancellation (or timeout) should change status of the socket since they are really independent. And I don't see any risk that we would loose data.

I added test for both sync & async versions.

Author:	wfurt
Assignees:	wfurt
Labels:	area-System.Net.Sockets
Milestone:	9.0.0

[tmds]

I don't see any risk that we would loose data.

The caller of ReadAsync won't loose data and can get it on the next call.

Note that a caller of ReadAtLeastAsync might loose the data that was already received when it was less than minimumBytes.

I personally don't see reason why the cancellation (or timeout) should change status of the socke

From a use-case perspective, for stream sockets, I think it would be unusual to cancel a read and then read again. The current behavior prevents that.

I don't think the current behavior is "limiting" for stream sockets. The cancellation of the read affects on-going sends (which will now be see the disconnected state). I don't think in practice this is much of a problem because the read cancellation is typically going to happen as part of disconnecting.

#69397 is reported for TCP (which is a stream socket) and the behavior issues that it lists should be handled by the other code that the the server/client should have, like disposing the socket on the client, and reading from the client socket on the server.

In general (also considering non-stream sockets), there may be cases where you want to have a usable socket after cancelling a read.

[cbworksdev]

From a use-case perspective, for stream sockets, I think it would be unusual to cancel a read and then read again. The current behavior prevents that.

I just want to add in that this is the behavior that I would desire to have. My use case is I want the socket to timeout because I have had network issues where the ReadAsync will wait forever because of some network issue. Having the socket timeout every 5 seconds (similar to how Read works with the timeout property set), allows me to run through my isConnected function which will check various things, and if its not connected, then initiate re connection. Just my 2 cents.

In general (also considering non-stream sockets), there may be cases where you want to have a usable socket after cancelling a read.

This is what @wfurt is asking for. But since the Connected flag goes to false, its unusable if you rely on that field.

[tmds]

allows me to run through my isConnected function which will check various things, and if its not connected, then initiate re connection.

What type of things do you check instead of reconnecting immediately on the timeout?

And if the checks say the socket is connected, you issue a new read on the socket that timed out for reading on the previous call?

[stephentoub]

If we're 100% sure that cancellation occurrence won't have impacted the socket in any way (e.g. no partial data will have been consumed and discarded), seems ok to leave it open.

[cbworksdev]

allows me to run through my isConnected function which will check various things, and if its not connected, then initiate re connection.

What type of things do you check instead of reconnecting immediately on the timeout?

And if the checks say the socket is connected, you issue a new read on the socket that timed out for reading on the previous call?

I would perform a non blocking read operation, possibly check the last received message timestamp, ect.

The issue is with ReadAsync I can't configure a timeout, so I have to use Read, or start managing multiple threads to handle this, which introduces potential issues.

This is all in the name of combating half open sockets.

[wfurt]

If we're 100% sure that cancellation occurrence won't have impacted the socket in any way (e.g. no partial data will have been consumed and discarded), seems ok to leave it open.

I'm fairly confident. Cancellation is .NET concept and as far as I know we don't call any socket function when it happens. e.g. it does not change state of the underlying socket.
Based on the concerns I'll beef up the tests to do more IO and verify correctness of the payload.

[stephentoub]

If we're 100% sure that cancellation occurrence won't have impacted the socket in any way (e.g. no partial data will have been consumed and discarded), seems ok to leave it open.

I'm fairly confident. Cancellation is .NET concept and as far as I know we don't call any socket function when it happens. e.g. it does not change state of the underlying socket. Based on the concerns I'll beef up the tests to do more IO and verify correctness of the payload.

What would happen, for example, if a WriteAsync were canceled during the write loop on unix after having performed a partial write of the data?

[wfurt]

What would happen, for example, if a WriteAsync were canceled during the write loop on unix after having performed a partial write of the data?

I did not check WriteAsync, my focus was on ReadAync. Now, since it throws, I think the operation state is undefined. When WriteAsync is cancelled there is no way to know if data were written or not. Keeping the Socket connected does not make it any worse IMHO.

For example event if WriteAsync is cancelled, you still may try to read data or vice versa. The Write/Send is more difficult anyway IMHO as the Write/Send may finish after data written to Socket buffer but hey are not delivered to the other side.

My goal here to make the synchronous and asynchronous path more similar. We have all the problem above already for synchronous path. Part of data may be written but then operation times out and the state is unknown.

I can also look into make this applicable only for read operations but I would personally find it strange.

[stephentoub]

Ok, that's reasonable.

[tmds]

I did not check WriteAsync, my focus was on ReadAync.

From diagonally reading through the code, I think the code that is changed here affects sending as well. And previously a cancelled partial send would disallow using the socket further, while with this change it will be allowed.

The current behavior is enforcing some things which seem reasonable for "stream"-type sockets.
By dropping this behavior, we do have some expectations of the user's behavior, like: don't try another send after cancelling a send.

I'm not opposed to the change, though I'm not sure what actual problem we're solving either.

[wfurt]

The current behavior is enforcing some things which seem reasonable for "stream"-type sockets.
By dropping this behavior, we do have some expectations of the user's behavior, like: don't try another send after cancelling a send.

That behavior already exists on NetworkStream with synchronous operations. If we strongly feel this should be limited to ReadAsync only I can do that but I would like to reach consensus first.

[stephentoub]

If I've understood correctly, the above test passes before this PR and the below test only passes after this PR?

[wfurt]

yes. This is because of the existing errorCode != SocketError.TimedOut.