Refactor Task Socket.ConnectAsync methods to use AwaitableSocketAsyncEventArgs #787
Conversation
|
@dotnet/ncl @stephentoub this PR is not finished but I'd like some early feedback. It seems github doesn't allow me to mark this as Draft and still do a CI run. Can you please mark this as no-merge? |
tmds
changed the title
tmds
force-pushed
the
socket_connectasync
branch
from
749388f
to
ffcc63a
Compare
MihaZupan
added
the
NO-MERGE
|
Thanks, @tmds. I'm concerned about doing something like this until https://github.com/dotnet/corefx/issues/39466 is addressed. |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
Although, looking at the change in more detail now, it seems you're not reusing the SAEA across sockets, only using it for that one socket? In which case maybe this isn't a big deal, because the SAEA will only be storing the same socket that it's being used with? |
src/libraries/System.Net.Sockets/tests/FunctionalTests/ArgumentValidationTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs
Outdated
Show resolved
Hide resolved
Yes, it's on the Socket where it was used to Connect, so not keeping another Socket alive. |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
stephentoub
removed
the
NO-MERGE
|
@tmds, some of these test failures look relevant. |
|
This test is now failing on Windows: [Fact]
public async Task Ctor_NotStream_ThrowsIOException()
{
using (Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp))
using (Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp))
{
listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
await client.ConnectAsync(new IPEndPoint(IPAddress.Loopback, ((IPEndPoint)listener.LocalEndPoint).Port));
Assert.Throws<IOException>(() => new NetworkStream(client));
}
}Instead of throwing at the expected location, it's throwing on the ConnectAsync call above: I'm missing the fallback for connectionless protocols that is part of BeginConnect: runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs Lines 2057 to 2074 in 3d7fcaf I'll add this and check if there are other things which may be missing. |
|
Thanks. |
tmds
force-pushed
the
socket_connectasync
branch
from
4745f62
to
6404854
Compare
|
The latest change was to change if (CanUseConnectEx(endPointSnapshot))
{
socketError = e.DoOperationConnectEx(this, _handle);
}
else
{
// For connectionless protocols, Connect is not an I/O call.
socketError = e.DoOperationConnect(this, _handle);
}I'm not sure if all parts of the
with stacktrace From the error message I'd guess I don't have a Windows machine to debug. |
@karelz can someone help me with this? |
@antonfirsov maybe you can have a look? |
|
@tmds your assumption was correct, This is even worse on master, since the original string path = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());
var endPoint = new UnixDomainSocketEndPoint(path);
using var server = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified);
using var client = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified);
server.Bind(endPoint);
server.Listen(1);
using var mre = new ManualResetEventSlim(false);
using var sea = new SocketAsyncEventArgs() {RemoteEndPoint = endPoint};
sea.Completed += (_, __) => mre.Set();
if (client.ConnectAsync(sea)) mre.Wait();
Assert.Equal(SocketError.Success, sea.SocketError);@stephentoub I wonder if I should create a separate issue for this or should we just add the fix to this PR for the sake of simplicity? |
|
A possible fix on my branch: I had to change the expected exception for the test case to My branch is also in sync with the current master, fixed two minor conflicts from #32675. @tmds can I update this PR with all these changes? Note that |
My preference would be to keep the bug fix and refactoring separate, ideally two PRs, but it could be two commits we don't squash if necessary. |
|
Thanks for having a look @antonfirsov! I hope Windows CI will now pass.
|
|
@tmds a few points: The Windows build is still failing in
Have you also tried to address this?
This means that we should enable this test now on Windows. Additionally, I was unable to find any confirmation for the old behavior in our docs, which supports my argument that this was simply a bug. Any chance you can create a separate bugfix PR as @stephentoub suggested? |
I'll rewrite the PR in 2 commits when it compiles and tests pass.
This is another Windows-specific problem? Can you take a look? I wonder what the SocketError is in |
|
@antonfirsov can you take a look at the remaining Windows issue? |
|
@tmds sure, was planning to do so today. |
Thank you @antonfirsov ! I have included these changes. When CI passes, I will put UDP Connect(saea) into a separate PR. |
|
CI shows unrelated test failures in And |
tmds
force-pushed
the
socket_connectasync
branch
from
c0f2b7a
to
e5d3ed1
Compare
| [InlineData(65536)] | ||
| public async Task ConnectAsync_IPAddresses_InvalidPort_Throws_ArgumentOutOfRange(int port) | ||
| { | ||
| await Assert.ThrowsAsync<ArgumentOutOfRangeException>(() => GetSocket().ConnectAsync(new[] { IPAddress.Loopback }, port)); |
There was a problem hiding this comment.
This requires await to throw. The check could be added in the Task ConnectAsync directly.
| using (var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp)) | ||
| { | ||
| socket.Bind(new IPEndPoint(IPAddress.Loopback, 0)); | ||
| socket.Listen(1); | ||
| Assert.Throws<InvalidOperationException>(() => { socket.ConnectAsync(new[] { IPAddress.Loopback }, 1); }); | ||
| await Assert.ThrowsAsync<InvalidOperationException>(() => socket.ConnectAsync(new[] { IPAddress.Loopback }, 1)); |
There was a problem hiding this comment.
This requires await to throw. The check could be added in the Task ConnectAsync directly.
| @@ -167,7 +164,7 @@ public async Task ConnectGetsCanceledByDispose() | |||
| disposedException = true; | |||
| } | |||
|
|
|||
| if (usesApm) | |||
| if (UsesApm) | |||
There was a problem hiding this comment.
The exception thrown by the Task ConnectAsync methods has changed. The exceptions are now consistent with other Task-returning Socket methods.
|
With #33674 merged, I've rebased this PR. I've added some comments with the test changes that indicate changed behavior. |
ghost
locked as resolved and limited conversation to collaborators
No description provided.