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.

Sign up for GitHub

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

Jump to bottom

Implement Equals & GetHashCode for LingerOption - fix Socket outerloop tests #78747

Merged
merged 2 commits into from
Nov 24, 2022
Merged

Implement Equals & GetHashCode for LingerOption - fix Socket outerloop tests #78747

merged 2 commits into from
Nov 24, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Nov 23, 2022

This is regression from #74645 & #73499
On Linux and macOS the test fail on Debug Assert

     System.Net.Sockets.Tests.DualModeConnectToDnsEndPoint.DualModeConnect_DnsEndPointToHost_Helper [SKIP]
        Condition(s) not met: "LocalhostIsBothIPv4AndIPv6"
  Process terminated. Assertion failed.
  LingerState. Expected: System.Net.Sockets.LingerOption, Actual: System.Net.Sockets.LingerOption
     at System.Net.Sockets.Socket.CopyStateFromSource(Socket source) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs:line 288
     at System.Net.Sockets.SocketAsyncEventArgs.FinishOperationAccept(SocketAddress remoteSocketAddress) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Unix.cs:line 348
     at System.Net.Sockets.SocketAsyncEventArgs.FinishOperationSyncSuccess(Int32 bytesTransferred, SocketFlags flags) in /home/furt/github/wfurt-

macOS additionally fails with exception as some of the gutters are not supported

     System.Net.Sockets.Tests.CreateSocket.Ctor_Raw_Supported_Success [SKIP]
        Condition(s) not met: "SupportsRawSockets"
  Unhandled exception. Unhandled exception. System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
   ---> System.Net.Sockets.SocketException (45): Operation not supported
     at System.Net.Sockets.Socket.UpdateStatusAfterSocketErrorAndThrowException(SocketError error, Boolean disconnectOnFailure, String callerName) in /Users/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 3660
     at System.Net.Sockets.Socket.UpdateStatusAfterSocketOptionErrorAndThrowException(SocketError error, String callerName) in /Users/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 3651
     at System.Net.Sockets.Socket.GetSocketOption(SocketOptionLevel optionLevel, SocketOptionName optionName) in /Users/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 2024
     at System.Net.Sockets.Socket.get_DontFragment() in /Users/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 642
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
     at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr) in /Users/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs:line 59
     --- End of inner exception stack trace ---

with this I can run tests with /p:outerloop=true once again on Unix

@wfurt wfurt requested review from antonfirsov and liveans November 23, 2022 06:31
@wfurt wfurt self-assigned this Nov 23, 2022
@ghost
Copy link

ghost commented Nov 23, 2022

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

Issue Details

This is regression from #74645 & #73499
On Linux and macOS the test fail on Debug Assert

     System.Net.Sockets.Tests.DualModeConnectToDnsEndPoint.DualModeConnect_DnsEndPointToHost_Helper [SKIP]
        Condition(s) not met: "LocalhostIsBothIPv4AndIPv6"
  Process terminated. Assertion failed.
  LingerState. Expected: System.Net.Sockets.LingerOption, Actual: System.Net.Sockets.LingerOption
     at System.Net.Sockets.Socket.CopyStateFromSource(Socket source) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs:line 288
     at System.Net.Sockets.SocketAsyncEventArgs.FinishOperationAccept(SocketAddress remoteSocketAddress) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Unix.cs:line 348
     at System.Net.Sockets.SocketAsyncEventArgs.FinishOperationSyncSuccess(Int32 bytesTransferred, SocketFlags flags) in /home/furt/github/wfurt-

macOS additionally fails with exception as some of the gutters are not supported

     System.Net.Sockets.Tests.CreateSocket.Ctor_Raw_Supported_Success [SKIP]
        Condition(s) not met: "SupportsRawSockets"
  Unhandled exception. Unhandled exception. System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
   ---> System.Net.Sockets.SocketException (45): Operation not supported
     at System.Net.Sockets.Socket.UpdateStatusAfterSocketErrorAndThrowException(SocketError error, Boolean disconnectOnFailure, String callerName) in /Users/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 3660
     at System.Net.Sockets.Socket.UpdateStatusAfterSocketOptionErrorAndThrowException(SocketError error, String callerName) in /Users/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 3651
     at System.Net.Sockets.Socket.GetSocketOption(SocketOptionLevel optionLevel, SocketOptionName optionName) in /Users/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 2024
     at System.Net.Sockets.Socket.get_DontFragment() in /Users/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 642
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
     at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr) in /Users/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs:line 59
     --- End of inner exception stack trace ---

with this I can run tests with /p:outerloop=true once again on Unix

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Sockets
Milestone: -

liveans
liveans approved these changes Nov 23, 2022
View reviewed changes
Copy link
Member

@liveans liveans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit:

antonfirsov
antonfirsov reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rename the PR, and it would be nice to understand better what is going wrong in the original variant. Looks good otherwise.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/LingerOption.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs
object? origValue = pi.GetValue(source);
object? cloneValue = pi.GetValue(this);

Debug.Assert(Equals(origValue, cloneValue), $"{pi.Name}. Expected: {origValue}, Actual: {cloneValue}");
Copy link
Member

@antonfirsov antonfirsov Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very weird that Equals(origValue, cloneValue) == false for LingerOption on Unix only. They should be equal by reference regardless of the platform.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @liveans knows the answer? Part of the difference is that Windows support Accept from socket natively while on Unix we construct it behind the scenes.

Copy link
Member

@liveans liveans Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that, when we create a new SafeSocketHandle for this purpose, we're not replacing public properties, we're just replacing private variables, that can be root cause of the issue. I'm going to raise a PR for this.

Edit: It turns out that wasn't the problem, but we need Equals function anyway, because we're creating new instance for LingerOption everytime we try to get value from LingerState.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR gets merged, I'll open a PR for transferring handle state between temporary and new handles for this case, as we already did here in ReplaceHandle method:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs

Lines 160 to 169 in d104e34

if (_handle.IsTrackedOption(TrackedSocketOptions.DualMode)) DualMode = _handle.DualMode;
if (_handle.IsTrackedOption(TrackedSocketOptions.DontFragment)) DontFragment = dontFragment;
if (_handle.IsTrackedOption(TrackedSocketOptions.EnableBroadcast)) EnableBroadcast = broadcast;
if (_handle.IsTrackedOption(TrackedSocketOptions.LingerState)) LingerState = linger!;
if (_handle.IsTrackedOption(TrackedSocketOptions.NoDelay)) NoDelay = noDelay;
if (_handle.IsTrackedOption(TrackedSocketOptions.ReceiveBufferSize)) ReceiveBufferSize = receiveSize;
if (_handle.IsTrackedOption(TrackedSocketOptions.ReceiveTimeout)) ReceiveTimeout = receiveTimeout;
if (_handle.IsTrackedOption(TrackedSocketOptions.SendBufferSize)) SendBufferSize = sendSize;
if (_handle.IsTrackedOption(TrackedSocketOptions.SendTimeout)) SendTimeout = sendTimeout;
if (_handle.IsTrackedOption(TrackedSocketOptions.Ttl)) Ttl = ttl;

@antonfirsov antonfirsov mentioned this pull request Nov 23, 2022
Merged
@wfurt @MihaZupan
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Linger…
c425624 
…Option.cs

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
This was referenced Nov 23, 2022
Open
Closed
@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Nov 24, 2022
Closed
@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member

Outerloop seems to be broken, innerloop failure is #69101

@antonfirsov antonfirsov changed the title fix Socket outerloop tests Implement Equals & GetHashCode for LingerOption - fix Socket outerloop tests Nov 24, 2022
@antonfirsov antonfirsov merged commit a230293 into dotnet:main Nov 24, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 25, 2022
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@liveans liveans liveans approved these changes

@MihaZupan MihaZupan MihaZupan left review comments

@antonfirsov antonfirsov antonfirsov approved these changes

Assignees

@wfurt wfurt

Labels
area-System.Net.Sockets
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@wfurt @antonfirsov @MihaZupan @liveans @karelz