Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plumb cancellation into Socket's implementation #23736

Closed
stephentoub opened this issue Oct 4, 2017 · 7 comments
Closed

Plumb cancellation into Socket's implementation #23736

stephentoub opened this issue Oct 4, 2017 · 7 comments
Assignees
Labels
area-System.Net.Sockets enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@stephentoub
Copy link
Member

#22919 adds public API to Socket types that accept CancellationToken. We can implement the APIs to just do an upfront check, but the real value is when cancellation is plumbed through to support canceling the actual I/O. This issue represents doing that work.

@toady
Copy link

toady commented Dec 1, 2017

Just a note for whoever resolves this:
NetworkStream.ReadAsync(byte[], int, int, CancellationToken)
is missing CancellationToken argument to underlying Socket.ReceiveAsync, yet it is present in
NetworkStream.ReadAsync(Memory<byte>, CancellationToken)

@WinCPP
Copy link
Contributor

WinCPP commented Dec 10, 2017

Hi @stephentoub further to our discussion in dotnet/corefx#25511 (here), I could pass down the CancellationToken all the way to SocketEventAsyncArgs.DoOperationReceive (here). However if my understanding is correct, this code area not being based on async-await pattern, it is not able to leverage CancellationToken. Hence I tried to convert it to async following async-all-the-way-(up) paradigm and I hit the interface function 'System.Net.Sockets.Socket.ReceiveAsync` (here) which needs to be made async as well, breaking the ApiCompat. And that tells me I'm not doing it in a correct way.

Would you like to see the code? Appreciate your inputs / hints on this one. Thanks!

@WinCPP
Copy link
Contributor

WinCPP commented Dec 10, 2017

Oh I just noticed, #23138 is also no more up-for-grabs :-) ... So should I be working on something else...? Thanks!

@Clockwork-Muse
Copy link
Contributor

(I'm the wrong Stephen, but...)

My first assumption is going to be that you're going to end up with different methods - a Task-returning async method likely won't take the SocketEventAsyncArgs type, instead preferring to manage the relevant info via the async state machine (or at least pretend to for callers). One thing I would have taken as an indication of this is the definition of the extension methods in #22919 .

Now, you're probably still going to want to use/manage a SocketEventAsyncArgs under the covers, if you can; the async state machine represents extra overhead, especially in situations where the operations may have already completed. TcpClient does something similar, if memory serves. This likely means you'll end up calling DoOperationReceive, but I haven't been in the code enough to definitively state what you need.

@karelz
Copy link
Member

karelz commented Mar 7, 2018

We won't be able to address this in 2.1, moving to Future.

@spiritedsnowcat
Copy link

What might be the ETA for fixing this issue?

@GSPP
Copy link

GSPP commented Aug 9, 2018

Adding two related issues:

@stephentoub stephentoub self-assigned this Mar 22, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Sockets enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

8 participants