-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix RHEL7 socket dispose hang, and extend coverage #43409
Changes from 15 commits
9f2c092
37176da
b7b8d2c
ee3dd19
48a69c4
512edf5
80501f9
4d83e73
ba5df5e
b301bb0
6158460
b77d31f
1da5006
5b9b879
102561a
f591786
6909cc8
8a3ce60
800d910
b371ae7
720c522
412a1df
86dca50
5b109e3
9842c81
1769820
7475c15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3085,6 +3085,11 @@ int32_t SystemNative_Disconnect(intptr_t socket) | |
addr.sa_family = AF_UNSPEC; | ||
|
||
err = connect(fd, &addr, sizeof(addr)); | ||
if (err != 0) | ||
{ | ||
// On some older kernels connect(AF_UNSPEC) may fail. Fall back to shutdown in these cases: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
err = shutdown(fd, SHUT_RDWR); | ||
} | ||
#elif HAVE_DISCONNECTX | ||
// disconnectx causes a FIN close on OSX. It's the best we can do. | ||
err = disconnectx(fd, SAE_ASSOCID_ANY, SAE_CONNID_ANY); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -289,16 +289,26 @@ public async Task AcceptAsync_MultipleAcceptsThenDispose_AcceptsThrowAfterDispos | |
} | ||
} | ||
|
||
[Fact] | ||
public async Task AcceptGetsCanceledByDispose() | ||
public static readonly TheoryData<IPAddress> AcceptGetsCanceledByDispose_Data = new TheoryData<IPAddress> | ||
{ | ||
{ IPAddress.Loopback }, | ||
{ IPAddress.IPv6Loopback }, | ||
{ IPAddress.Loopback.MapToIPv6() } | ||
}; | ||
|
||
[Theory] | ||
[MemberData(nameof(AcceptGetsCanceledByDispose_Data))] | ||
public async Task AcceptGetsCanceledByDispose(IPAddress loopback) | ||
{ | ||
// We try this a couple of times to deal with a timing race: if the Dispose happens | ||
// before the operation is started, we won't see a SocketException. | ||
int msDelay = 100; | ||
await RetryHelper.ExecuteAsync(async () => | ||
int retries = 10; | ||
while (true) | ||
{ | ||
var listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); | ||
listener.Bind(new IPEndPoint(IPAddress.Loopback, 0)); | ||
var listener = new Socket(loopback.AddressFamily, SocketType.Stream, ProtocolType.Tcp); | ||
if (loopback.IsIPv4MappedToIPv6) listener.DualMode = true; | ||
listener.Bind(new IPEndPoint(loopback, 0)); | ||
listener.Listen(1); | ||
|
||
Task acceptTask = AcceptAsync(listener); | ||
|
@@ -330,20 +340,33 @@ await RetryHelper.ExecuteAsync(async () => | |
disposedException = true; | ||
} | ||
|
||
if (UsesApm) | ||
{ | ||
Assert.Null(localSocketError); | ||
Assert.True(disposedException); | ||
} | ||
else if (UsesSync) | ||
try | ||
{ | ||
Assert.Equal(SocketError.Interrupted, localSocketError); | ||
if (UsesApm) | ||
{ | ||
Assert.Null(localSocketError); | ||
Assert.True(disposedException); | ||
} | ||
else if (UsesSync) | ||
{ | ||
Assert.Equal(SocketError.Interrupted, localSocketError); | ||
} | ||
else | ||
{ | ||
Assert.Equal(SocketError.OperationAborted, localSocketError); | ||
} | ||
break; | ||
} | ||
else | ||
catch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we catch a specific exception here, like some XUnit exception class? Seems like that would be safer and clearer. Similarly below. |
||
{ | ||
Assert.Equal(SocketError.OperationAborted, localSocketError); | ||
if (retries-- > 0) | ||
{ | ||
continue; | ||
} | ||
|
||
throw; | ||
} | ||
}, maxAttempts: 10); | ||
} | ||
} | ||
|
||
[Fact] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: these are not
client
andserver
, but two connected TCP peers. Theserver
Socket doesn't leave the method, and it looks like we are not Disposing it, which we probably should.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The terminology reflects their roles during the establishment of the connection. I understand that this information is not needed afterwards, but I still find the labels useful for convenience, because it helps me mentally map the tests to a common real-world scenarios. For example
dualModeClient = false
refers to the firstSocket client
. and it sounds more natural thanbool dualModeSocket1
, which would be the case if we'd rename the tuple. We use this terminology in other tests, although it's not necessary or meaningful to label anything as client/server. This is not a very strong opinion although.You mean the
listner
I guess. I also don't like this, but this problem is not specific to the PR. We should fix it on all call sites which I would do separately.