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

Sockets: Reimplement remaining Task-based async methods using SocketAsyncEventArgs #41502

Closed
geoffkizer opened this issue Aug 28, 2020 · 12 comments · Fixed by #47229
Closed

Sockets: Reimplement remaining Task-based async methods using SocketAsyncEventArgs #41502

geoffkizer opened this issue Aug 28, 2020 · 12 comments · Fixed by #47229
Assignees
Milestone

Comments

@geoffkizer
Copy link
Contributor

Most socket Task-based async APIs are implemented using SocketAsyncEventArgs under the covers today.

The few that are not include:

SendToAsync
ReceiveFromAsync
ReceiveMessageFromAsync

These are instead implemented by wrapping the old IAsyncResult-based async methods.

We should change these to use SocketAsyncEventArgs instead.

@ghost
Copy link

ghost commented Aug 28, 2020

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

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Aug 28, 2020
@scalablecory scalablecory removed the untriaged New issue has not been triaged by the area owner label Aug 30, 2020
@antonfirsov
Copy link
Member

When doing so, we need to review the exceptions thrown by different implementations, and either decide to harmonize (-> break and document) or to keep compatibility. See: #41585 (comment)

@geoffkizer geoffkizer self-assigned this Sep 15, 2020
@antonfirsov
Copy link
Member

@geoffkizer are you actually working on this? If not, I can give it a try.

@geoffkizer
Copy link
Contributor Author

I have a PR in progress for SendTo/ReceiveFrom/ReceiveMessageFrom. I haven't had time to look at it recently. If you'd like to take what I've got and finish it up, you are welcome to.

The other thing we should do here is to audit the code to see if there are any other methods besides this that are still using old IAsyncResult logic under the covers. I know the MultiConnect stuff was, but @stephentoub just fixed this. I think SendFile is another place -- see #42591.

There may be others as well, I'm not entirely sure...

@stephentoub
Copy link
Member

stephentoub commented Oct 26, 2020

but @stephentoub just fixed this

Hasn't quite been merged yet, but will soon.

@geoffkizer
Copy link
Contributor Author

Just looked: DisconnectAsync doesn't have a Task-based version either (like SendFile). We need to add this.

@stephentoub
Copy link
Member

stephentoub commented Oct 26, 2020

Just looked: DisconnectAsync doesn't have a Task-based version either (like SendFile). We need to add this.

FYI, that's called out in #33417. It had been its own issue at #1608.

@geoffkizer
Copy link
Contributor Author

The other thing we should be looking at here is getting rid of the old IAsyncResult implementations and replacing them with wrappers on top of Task APIs. We should be able to do this for the APIs that already have Task implementations, i.e. Connect/Accept/Send/Receive.

I thought there was an issue on this but I can't find it at the moment. Will add if necessary.

@geoffkizer
Copy link
Contributor Author

FYI, that's called out in #33417.

Yeah, unfortunately, that issue hasn't had API review. I will probably file a separate issue for DisconnectAsync specifically so we can get it through API review quickly.

@stephentoub
Copy link
Member

I will probably file a separate issue for DisconnectAsync specifically so we can get it through API review quickly.

Ok, maybe just reopen the original issue #1608.

@geoffkizer
Copy link
Contributor Author

Ok, maybe just reopen the original issue #1608.

Done

@antonfirsov
Copy link
Member

antonfirsov commented Nov 3, 2020

I think we should fix #1712 (harden and re-enable test SendReceive.SendToRecvFrom_Datagram_UDP) before changing anything in SendTo/ReceiveFrom.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 20, 2021
antonfirsov added a commit that referenced this issue Jan 27, 2021
…sing SocketAsyncEventArgs (#47229)

Closes #41502, but does not change the existing APM methods
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 27, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants