Add cache for Socket.LocalEndPoint

[CarnaViire]

Added _localEndPoint field to cache LocalEndPoint value. Introduced RightEndPoint property to encapsulate clearing _localEndPoint on _rightEndPoint change.Cached value is cleared on connect, error and disconnect. Cached value is cleared on error and disconnect. If the value is a wildcard address, it is also cleared on connect and accept.

Fix#1482

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[karelz]

LGTM, but I let @antonfirsov make the final ok ;)

[antonfirsov]

I'm concerned if we should maybe reset _localEndpoint to null in methods where we also reset _rightEndpoint:

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

Lines 2685 to 2712 in f8e92ec

	EndPoint?oldEndPoint=_rightEndPoint;
	// Guarantee to call CheckAsyncCallOverlappedResult if we call SetUnamangedStructures with a cache in order to
	// avoid a Socket leak in case of error.
	SocketErrorerrorCode=SocketError.SocketError;
	try
	{
	if(_rightEndPoint==null)
	{
	_rightEndPoint=endPointSnapshot;
	}
	errorCode=SocketPal.SendToAsync(_handle,buffer,offset,size,socketFlags,socketAddress,asyncResult);
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SendToAsync returns:{errorCode} size:{size} returning AsyncResult:{asyncResult}");
	}
	catch(ObjectDisposedException)
	{
	_rightEndPoint=oldEndPoint;
	throw;
	}
	// Throw an appropriate SocketException if the native call fails synchronously.
	if(!CheckErrorAndUpdateStatus(errorCode))
	{
	UpdateSendSocketErrorForDisposed(referrorCode);
	// Update the internal state of this socket according to the error before throwing.
	_rightEndPoint=oldEndPoint;

If I'm not missing anything there will be no user callback querying LocalEndpoint in case of failure, however we may end up adding internal code doing so in the future, for example in a tracing call like this:

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

Line 1083 in f8e92ec

if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint}");

Such a call may enforce caching a wrong value for LocalEndpoint permanently, on the other hand it's probably very unlikely we do such a thing by accident. I wonder if it makes sense to add tests to ensure such failing async methods will not alter the sockets internal state in an unwanted way.

@tmds maybe you have some thoughts on this?

[CarnaViire]

@antonfirsov Here is a list of all changes of _rightEndPoint that I have found in the code (sorry for the long list, I would put it under cut if I knew how)

1. setting to new value

   1.1. in constructor
   Socket.cs(165:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(173:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(180:25): _rightEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress));
   Socket.Windows.cs(69:17): _rightEndPoint = ep.Create(socketAddress);

   1.2. after non-blocking connect
   [LocalEndPoint] Socket.cs(319:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [RemoteEndPoint] Socket.cs(361:25): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [Connected] Socket.cs(472:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;

   1.3. if it was null before
   [DoBind] Socket.cs(834:17): _rightEndPoint = endPointSnapshot;
   [SendTo] Socket.cs(1329:17): _rightEndPoint = remoteEP;
   [ReceiveMessageFrom] Socket.cs(1566:21): _rightEndPoint = endPointSnapshot;
   [ReceiveFrom] Socket.cs(1650:21): _rightEndPoint = endPointSnapshot;
   [DoBeginSendTo] Socket.cs(2694:21): _rightEndPoint = endPointSnapshot;
   [BeginReceiveMessageFrom] Socket.cs(3059:21): _rightEndPoint = remoteEP;
   [DoBeginReceiveFrom] Socket.cs(3276:21): _rightEndPoint = endPointSnapshot;
   [ConnectAsync] Socket.cs(3677:21): _rightEndPoint = endPointSnapshot;
   [SendToAsync] Socket.cs(4019:17): _rightEndPoint = endPointSnapshot;
   [DoConnect] Socket.cs(4155:17): _rightEndPoint = endPointSnapshot;
   [BeginConnectEx] Socket.cs(4536:17): _rightEndPoint = endPointSnapshot;

   1.4. on inheriting other socket state
   [UpdateAcceptSocket] Socket.cs(4784:20): socket._rightEndPoint = _rightEndPoint;

2. setting to previous value - on some error

   2.1. on ObjectDisposedException, throwing later
   [DoBeginSendTo] Socket.cs(2703:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3081:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3285:17): _rightEndPoint = oldEndPoint;

   2.2. on any exception, throwing later
   [ConnectAsync] Socket.cs(3700:21): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4547:17): _rightEndPoint = oldEndPoint;

   2.3. on false CheckErrorAndUpdateStatus, throwing later
   [DoBeginSendTo] Socket.cs(2712:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3090:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3294:17): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4563:17): _rightEndPoint = oldEndPoint;

   2.4. on false CheckErrorAndUpdateStatus, NO throwing
   [SendToAsync] Socket.cs(4037:17): _rightEndPoint = oldEndPoint;

3. setting to null - on some error

   3.1. on any exception, throwing later
   [SendToAsync] Socket.cs(4029:17): _rightEndPoint = null;

You suggest to clear _localEndPoint on the reset of _rightEndPoint, which are items 2. and 3. on the list. If we look at subitems of item 1., we can see that most of them are if _rightEndPoint was null before or in the constructor, which means if we clear _localEndPoint at this time we won't actually change anything. And a change of _rightEndPoint after non-blocking connect will only happen once I believe.

What I am trying to say is if we want to clear _localEndPoint on the reset of _rightEndPoint, maybe we could actually return to my original proposal of clearing _localEndPoint on every _rightEndPoint change? The only place where it might possibly unnesessary clear some cached value is this non-blocking connect part (item 1.2.) and we might use _rightEndPoint there instead of new property RightEndPoint specifically in this case.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

So the behavior with wildcard addresses is as follows.

1. If a TCP server socket (listener socket) was bound to a wildcard address, then the accept socket created with accept will be assigned with a specific address based on what interface a client socket connected to. That means we can't copy cached listener's local endpoint if it is wildcard. However, if the listener was bound to a specific address, we can copy that to the accept socket freely.

2. If a TCP client socket was bound to a wildcard address, then it will be assigned with a specific address after connect. That means we should clear the client socket's cached local endpoint if it is wildcard.

3. If an UDP socket was bound to a wildcard address, it stays as wildcard even after sendto and other calls.

I added clearing of cached wildcard local endpoints on connect and accept with clarifying comments. Also added tests that illustrate this behavior.

cc @geoffkizer@antonfirsov

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Outerloop test failures are #40926 and #41381

[antonfirsov]

We need to make sure all independent implementations are covered in the tests, otherwise looks good now.

[antonfirsov]

These 2 tests seem to test the same thing for two different connect implementations. I suggest dedupe their code and also cover the remaining ones: ConnectAsync(SocketAsyncEventArgs) and SocketTaskExtensions.ConnectAsync(this Socket, ....). For this the best would be to utilize SocketTestHelperBase<T>.

[CarnaViire]

Rewrote the tests, please check that I understood you correctly :)

[CarnaViire]

Added _localEndPoint field to cache LocalEndPoint value. Introduced RightEndPoint property to encapsulate clearing _localEndPoint on _rightEndPoint change.Cached value is cleared on connect, error and disconnect. Cached value is cleared on error and disconnect. If the value is a wildcard address, it is also cleared on connect and accept.

Fix#1482

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[karelz]

LGTM, but I let @antonfirsov make the final ok ;)

[antonfirsov]

I'm concerned if we should maybe reset _localEndpoint to null in methods where we also reset _rightEndpoint:

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

Lines 2685 to 2712 in f8e92ec

	EndPoint?oldEndPoint=_rightEndPoint;
	// Guarantee to call CheckAsyncCallOverlappedResult if we call SetUnamangedStructures with a cache in order to
	// avoid a Socket leak in case of error.
	SocketErrorerrorCode=SocketError.SocketError;
	try
	{
	if(_rightEndPoint==null)
	{
	_rightEndPoint=endPointSnapshot;
	}
	errorCode=SocketPal.SendToAsync(_handle,buffer,offset,size,socketFlags,socketAddress,asyncResult);
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SendToAsync returns:{errorCode} size:{size} returning AsyncResult:{asyncResult}");
	}
	catch(ObjectDisposedException)
	{
	_rightEndPoint=oldEndPoint;
	throw;
	}
	// Throw an appropriate SocketException if the native call fails synchronously.
	if(!CheckErrorAndUpdateStatus(errorCode))
	{
	UpdateSendSocketErrorForDisposed(referrorCode);
	// Update the internal state of this socket according to the error before throwing.
	_rightEndPoint=oldEndPoint;

If I'm not missing anything there will be no user callback querying LocalEndpoint in case of failure, however we may end up adding internal code doing so in the future, for example in a tracing call like this:

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

Line 1083 in f8e92ec

if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint}");

Such a call may enforce caching a wrong value for LocalEndpoint permanently, on the other hand it's probably very unlikely we do such a thing by accident. I wonder if it makes sense to add tests to ensure such failing async methods will not alter the sockets internal state in an unwanted way.

@tmds maybe you have some thoughts on this?

[CarnaViire]

@antonfirsov Here is a list of all changes of _rightEndPoint that I have found in the code (sorry for the long list, I would put it under cut if I knew how)

1. setting to new value

   1.1. in constructor
   Socket.cs(165:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(173:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(180:25): _rightEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress));
   Socket.Windows.cs(69:17): _rightEndPoint = ep.Create(socketAddress);

   1.2. after non-blocking connect
   [LocalEndPoint] Socket.cs(319:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [RemoteEndPoint] Socket.cs(361:25): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [Connected] Socket.cs(472:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;

   1.3. if it was null before
   [DoBind] Socket.cs(834:17): _rightEndPoint = endPointSnapshot;
   [SendTo] Socket.cs(1329:17): _rightEndPoint = remoteEP;
   [ReceiveMessageFrom] Socket.cs(1566:21): _rightEndPoint = endPointSnapshot;
   [ReceiveFrom] Socket.cs(1650:21): _rightEndPoint = endPointSnapshot;
   [DoBeginSendTo] Socket.cs(2694:21): _rightEndPoint = endPointSnapshot;
   [BeginReceiveMessageFrom] Socket.cs(3059:21): _rightEndPoint = remoteEP;
   [DoBeginReceiveFrom] Socket.cs(3276:21): _rightEndPoint = endPointSnapshot;
   [ConnectAsync] Socket.cs(3677:21): _rightEndPoint = endPointSnapshot;
   [SendToAsync] Socket.cs(4019:17): _rightEndPoint = endPointSnapshot;
   [DoConnect] Socket.cs(4155:17): _rightEndPoint = endPointSnapshot;
   [BeginConnectEx] Socket.cs(4536:17): _rightEndPoint = endPointSnapshot;

   1.4. on inheriting other socket state
   [UpdateAcceptSocket] Socket.cs(4784:20): socket._rightEndPoint = _rightEndPoint;

2. setting to previous value - on some error

   2.1. on ObjectDisposedException, throwing later
   [DoBeginSendTo] Socket.cs(2703:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3081:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3285:17): _rightEndPoint = oldEndPoint;

   2.2. on any exception, throwing later
   [ConnectAsync] Socket.cs(3700:21): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4547:17): _rightEndPoint = oldEndPoint;

   2.3. on false CheckErrorAndUpdateStatus, throwing later
   [DoBeginSendTo] Socket.cs(2712:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3090:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3294:17): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4563:17): _rightEndPoint = oldEndPoint;

   2.4. on false CheckErrorAndUpdateStatus, NO throwing
   [SendToAsync] Socket.cs(4037:17): _rightEndPoint = oldEndPoint;

3. setting to null - on some error

   3.1. on any exception, throwing later
   [SendToAsync] Socket.cs(4029:17): _rightEndPoint = null;

You suggest to clear _localEndPoint on the reset of _rightEndPoint, which are items 2. and 3. on the list. If we look at subitems of item 1., we can see that most of them are if _rightEndPoint was null before or in the constructor, which means if we clear _localEndPoint at this time we won't actually change anything. And a change of _rightEndPoint after non-blocking connect will only happen once I believe.

What I am trying to say is if we want to clear _localEndPoint on the reset of _rightEndPoint, maybe we could actually return to my original proposal of clearing _localEndPoint on every _rightEndPoint change? The only place where it might possibly unnesessary clear some cached value is this non-blocking connect part (item 1.2.) and we might use _rightEndPoint there instead of new property RightEndPoint specifically in this case.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

So the behavior with wildcard addresses is as follows.

1. If a TCP server socket (listener socket) was bound to a wildcard address, then the accept socket created with accept will be assigned with a specific address based on what interface a client socket connected to. That means we can't copy cached listener's local endpoint if it is wildcard. However, if the listener was bound to a specific address, we can copy that to the accept socket freely.

2. If a TCP client socket was bound to a wildcard address, then it will be assigned with a specific address after connect. That means we should clear the client socket's cached local endpoint if it is wildcard.

3. If an UDP socket was bound to a wildcard address, it stays as wildcard even after sendto and other calls.

I added clearing of cached wildcard local endpoints on connect and accept with clarifying comments. Also added tests that illustrate this behavior.

cc @geoffkizer@antonfirsov

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Outerloop test failures are #40926 and #41381

[antonfirsov]

We need to make sure all independent implementations are covered in the tests, otherwise looks good now.

[antonfirsov]

These 2 tests seem to test the same thing for two different connect implementations. I suggest dedupe their code and also cover the remaining ones: ConnectAsync(SocketAsyncEventArgs) and SocketTaskExtensions.ConnectAsync(this Socket, ....). For this the best would be to utilize SocketTestHelperBase<T>.

[CarnaViire]

Rewrote the tests, please check that I understood you correctly :)

[CarnaViire]

Added _localEndPoint field to cache LocalEndPoint value. Introduced RightEndPoint property to encapsulate clearing _localEndPoint on _rightEndPoint change.Cached value is cleared on connect, error and disconnect. Cached value is cleared on error and disconnect. If the value is a wildcard address, it is also cleared on connect and accept.

Fix#1482

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[karelz]

LGTM, but I let @antonfirsov make the final ok ;)

[antonfirsov]

I'm concerned if we should maybe reset _localEndpoint to null in methods where we also reset _rightEndpoint:

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

Lines 2685 to 2712 in f8e92ec

	EndPoint?oldEndPoint=_rightEndPoint;
	// Guarantee to call CheckAsyncCallOverlappedResult if we call SetUnamangedStructures with a cache in order to
	// avoid a Socket leak in case of error.
	SocketErrorerrorCode=SocketError.SocketError;
	try
	{
	if(_rightEndPoint==null)
	{
	_rightEndPoint=endPointSnapshot;
	}
	errorCode=SocketPal.SendToAsync(_handle,buffer,offset,size,socketFlags,socketAddress,asyncResult);
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SendToAsync returns:{errorCode} size:{size} returning AsyncResult:{asyncResult}");
	}
	catch(ObjectDisposedException)
	{
	_rightEndPoint=oldEndPoint;
	throw;
	}
	// Throw an appropriate SocketException if the native call fails synchronously.
	if(!CheckErrorAndUpdateStatus(errorCode))
	{
	UpdateSendSocketErrorForDisposed(referrorCode);
	// Update the internal state of this socket according to the error before throwing.
	_rightEndPoint=oldEndPoint;

If I'm not missing anything there will be no user callback querying LocalEndpoint in case of failure, however we may end up adding internal code doing so in the future, for example in a tracing call like this:

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

Line 1083 in f8e92ec

if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint}");

Such a call may enforce caching a wrong value for LocalEndpoint permanently, on the other hand it's probably very unlikely we do such a thing by accident. I wonder if it makes sense to add tests to ensure such failing async methods will not alter the sockets internal state in an unwanted way.

@tmds maybe you have some thoughts on this?

[CarnaViire]

@antonfirsov Here is a list of all changes of _rightEndPoint that I have found in the code (sorry for the long list, I would put it under cut if I knew how)

1. setting to new value

   1.1. in constructor
   Socket.cs(165:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(173:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(180:25): _rightEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress));
   Socket.Windows.cs(69:17): _rightEndPoint = ep.Create(socketAddress);

   1.2. after non-blocking connect
   [LocalEndPoint] Socket.cs(319:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [RemoteEndPoint] Socket.cs(361:25): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [Connected] Socket.cs(472:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;

   1.3. if it was null before
   [DoBind] Socket.cs(834:17): _rightEndPoint = endPointSnapshot;
   [SendTo] Socket.cs(1329:17): _rightEndPoint = remoteEP;
   [ReceiveMessageFrom] Socket.cs(1566:21): _rightEndPoint = endPointSnapshot;
   [ReceiveFrom] Socket.cs(1650:21): _rightEndPoint = endPointSnapshot;
   [DoBeginSendTo] Socket.cs(2694:21): _rightEndPoint = endPointSnapshot;
   [BeginReceiveMessageFrom] Socket.cs(3059:21): _rightEndPoint = remoteEP;
   [DoBeginReceiveFrom] Socket.cs(3276:21): _rightEndPoint = endPointSnapshot;
   [ConnectAsync] Socket.cs(3677:21): _rightEndPoint = endPointSnapshot;
   [SendToAsync] Socket.cs(4019:17): _rightEndPoint = endPointSnapshot;
   [DoConnect] Socket.cs(4155:17): _rightEndPoint = endPointSnapshot;
   [BeginConnectEx] Socket.cs(4536:17): _rightEndPoint = endPointSnapshot;

   1.4. on inheriting other socket state
   [UpdateAcceptSocket] Socket.cs(4784:20): socket._rightEndPoint = _rightEndPoint;

2. setting to previous value - on some error

   2.1. on ObjectDisposedException, throwing later
   [DoBeginSendTo] Socket.cs(2703:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3081:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3285:17): _rightEndPoint = oldEndPoint;

   2.2. on any exception, throwing later
   [ConnectAsync] Socket.cs(3700:21): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4547:17): _rightEndPoint = oldEndPoint;

   2.3. on false CheckErrorAndUpdateStatus, throwing later
   [DoBeginSendTo] Socket.cs(2712:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3090:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3294:17): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4563:17): _rightEndPoint = oldEndPoint;

   2.4. on false CheckErrorAndUpdateStatus, NO throwing
   [SendToAsync] Socket.cs(4037:17): _rightEndPoint = oldEndPoint;

3. setting to null - on some error

   3.1. on any exception, throwing later
   [SendToAsync] Socket.cs(4029:17): _rightEndPoint = null;

You suggest to clear _localEndPoint on the reset of _rightEndPoint, which are items 2. and 3. on the list. If we look at subitems of item 1., we can see that most of them are if _rightEndPoint was null before or in the constructor, which means if we clear _localEndPoint at this time we won't actually change anything. And a change of _rightEndPoint after non-blocking connect will only happen once I believe.

What I am trying to say is if we want to clear _localEndPoint on the reset of _rightEndPoint, maybe we could actually return to my original proposal of clearing _localEndPoint on every _rightEndPoint change? The only place where it might possibly unnesessary clear some cached value is this non-blocking connect part (item 1.2.) and we might use _rightEndPoint there instead of new property RightEndPoint specifically in this case.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

So the behavior with wildcard addresses is as follows.

1. If a TCP server socket (listener socket) was bound to a wildcard address, then the accept socket created with accept will be assigned with a specific address based on what interface a client socket connected to. That means we can't copy cached listener's local endpoint if it is wildcard. However, if the listener was bound to a specific address, we can copy that to the accept socket freely.

2. If a TCP client socket was bound to a wildcard address, then it will be assigned with a specific address after connect. That means we should clear the client socket's cached local endpoint if it is wildcard.

3. If an UDP socket was bound to a wildcard address, it stays as wildcard even after sendto and other calls.

I added clearing of cached wildcard local endpoints on connect and accept with clarifying comments. Also added tests that illustrate this behavior.

cc @geoffkizer@antonfirsov

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Outerloop test failures are #40926 and #41381

[antonfirsov]

We need to make sure all independent implementations are covered in the tests, otherwise looks good now.

[antonfirsov]

These 2 tests seem to test the same thing for two different connect implementations. I suggest dedupe their code and also cover the remaining ones: ConnectAsync(SocketAsyncEventArgs) and SocketTaskExtensions.ConnectAsync(this Socket, ....). For this the best would be to utilize SocketTestHelperBase<T>.

[CarnaViire]

Rewrote the tests, please check that I understood you correctly :)

[CarnaViire]

Added _localEndPoint field to cache LocalEndPoint value. Introduced RightEndPoint property to encapsulate clearing _localEndPoint on _rightEndPoint change.Cached value is cleared on connect, error and disconnect. Cached value is cleared on error and disconnect. If the value is a wildcard address, it is also cleared on connect and accept.

Fix#1482

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[karelz]

LGTM, but I let @antonfirsov make the final ok ;)

[antonfirsov]

I'm concerned if we should maybe reset _localEndpoint to null in methods where we also reset _rightEndpoint:

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

Lines 2685 to 2712 in f8e92ec

	EndPoint?oldEndPoint=_rightEndPoint;
	// Guarantee to call CheckAsyncCallOverlappedResult if we call SetUnamangedStructures with a cache in order to
	// avoid a Socket leak in case of error.
	SocketErrorerrorCode=SocketError.SocketError;
	try
	{
	if(_rightEndPoint==null)
	{
	_rightEndPoint=endPointSnapshot;
	}
	errorCode=SocketPal.SendToAsync(_handle,buffer,offset,size,socketFlags,socketAddress,asyncResult);
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SendToAsync returns:{errorCode} size:{size} returning AsyncResult:{asyncResult}");
	}
	catch(ObjectDisposedException)
	{
	_rightEndPoint=oldEndPoint;
	throw;
	}
	// Throw an appropriate SocketException if the native call fails synchronously.
	if(!CheckErrorAndUpdateStatus(errorCode))
	{
	UpdateSendSocketErrorForDisposed(referrorCode);
	// Update the internal state of this socket according to the error before throwing.
	_rightEndPoint=oldEndPoint;

If I'm not missing anything there will be no user callback querying LocalEndpoint in case of failure, however we may end up adding internal code doing so in the future, for example in a tracing call like this:

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

Line 1083 in f8e92ec

if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint}");

Such a call may enforce caching a wrong value for LocalEndpoint permanently, on the other hand it's probably very unlikely we do such a thing by accident. I wonder if it makes sense to add tests to ensure such failing async methods will not alter the sockets internal state in an unwanted way.

@tmds maybe you have some thoughts on this?

[CarnaViire]

@antonfirsov Here is a list of all changes of _rightEndPoint that I have found in the code (sorry for the long list, I would put it under cut if I knew how)

1. setting to new value

   1.1. in constructor
   Socket.cs(165:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(173:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(180:25): _rightEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress));
   Socket.Windows.cs(69:17): _rightEndPoint = ep.Create(socketAddress);

   1.2. after non-blocking connect
   [LocalEndPoint] Socket.cs(319:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [RemoteEndPoint] Socket.cs(361:25): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [Connected] Socket.cs(472:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;

   1.3. if it was null before
   [DoBind] Socket.cs(834:17): _rightEndPoint = endPointSnapshot;
   [SendTo] Socket.cs(1329:17): _rightEndPoint = remoteEP;
   [ReceiveMessageFrom] Socket.cs(1566:21): _rightEndPoint = endPointSnapshot;
   [ReceiveFrom] Socket.cs(1650:21): _rightEndPoint = endPointSnapshot;
   [DoBeginSendTo] Socket.cs(2694:21): _rightEndPoint = endPointSnapshot;
   [BeginReceiveMessageFrom] Socket.cs(3059:21): _rightEndPoint = remoteEP;
   [DoBeginReceiveFrom] Socket.cs(3276:21): _rightEndPoint = endPointSnapshot;
   [ConnectAsync] Socket.cs(3677:21): _rightEndPoint = endPointSnapshot;
   [SendToAsync] Socket.cs(4019:17): _rightEndPoint = endPointSnapshot;
   [DoConnect] Socket.cs(4155:17): _rightEndPoint = endPointSnapshot;
   [BeginConnectEx] Socket.cs(4536:17): _rightEndPoint = endPointSnapshot;

   1.4. on inheriting other socket state
   [UpdateAcceptSocket] Socket.cs(4784:20): socket._rightEndPoint = _rightEndPoint;

2. setting to previous value - on some error

   2.1. on ObjectDisposedException, throwing later
   [DoBeginSendTo] Socket.cs(2703:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3081:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3285:17): _rightEndPoint = oldEndPoint;

   2.2. on any exception, throwing later
   [ConnectAsync] Socket.cs(3700:21): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4547:17): _rightEndPoint = oldEndPoint;

   2.3. on false CheckErrorAndUpdateStatus, throwing later
   [DoBeginSendTo] Socket.cs(2712:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3090:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3294:17): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4563:17): _rightEndPoint = oldEndPoint;

   2.4. on false CheckErrorAndUpdateStatus, NO throwing
   [SendToAsync] Socket.cs(4037:17): _rightEndPoint = oldEndPoint;

3. setting to null - on some error

   3.1. on any exception, throwing later
   [SendToAsync] Socket.cs(4029:17): _rightEndPoint = null;

You suggest to clear _localEndPoint on the reset of _rightEndPoint, which are items 2. and 3. on the list. If we look at subitems of item 1., we can see that most of them are if _rightEndPoint was null before or in the constructor, which means if we clear _localEndPoint at this time we won't actually change anything. And a change of _rightEndPoint after non-blocking connect will only happen once I believe.

What I am trying to say is if we want to clear _localEndPoint on the reset of _rightEndPoint, maybe we could actually return to my original proposal of clearing _localEndPoint on every _rightEndPoint change? The only place where it might possibly unnesessary clear some cached value is this non-blocking connect part (item 1.2.) and we might use _rightEndPoint there instead of new property RightEndPoint specifically in this case.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

So the behavior with wildcard addresses is as follows.

1. If a TCP server socket (listener socket) was bound to a wildcard address, then the accept socket created with accept will be assigned with a specific address based on what interface a client socket connected to. That means we can't copy cached listener's local endpoint if it is wildcard. However, if the listener was bound to a specific address, we can copy that to the accept socket freely.

2. If a TCP client socket was bound to a wildcard address, then it will be assigned with a specific address after connect. That means we should clear the client socket's cached local endpoint if it is wildcard.

3. If an UDP socket was bound to a wildcard address, it stays as wildcard even after sendto and other calls.

I added clearing of cached wildcard local endpoints on connect and accept with clarifying comments. Also added tests that illustrate this behavior.

cc @geoffkizer@antonfirsov

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Outerloop test failures are #40926 and #41381

[antonfirsov]

We need to make sure all independent implementations are covered in the tests, otherwise looks good now.

[antonfirsov]

These 2 tests seem to test the same thing for two different connect implementations. I suggest dedupe their code and also cover the remaining ones: ConnectAsync(SocketAsyncEventArgs) and SocketTaskExtensions.ConnectAsync(this Socket, ....). For this the best would be to utilize SocketTestHelperBase<T>.

[CarnaViire]

Rewrote the tests, please check that I understood you correctly :)

[CarnaViire]

Added _localEndPoint field to cache LocalEndPoint value. Introduced RightEndPoint property to encapsulate clearing _localEndPoint on _rightEndPoint change.Cached value is cleared on connect, error and disconnect. Cached value is cleared on error and disconnect. If the value is a wildcard address, it is also cleared on connect and accept.

Fix#1482

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[karelz]

LGTM, but I let @antonfirsov make the final ok ;)

[antonfirsov]

I'm concerned if we should maybe reset _localEndpoint to null in methods where we also reset _rightEndpoint:

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

Lines 2685 to 2712 in f8e92ec

	EndPoint?oldEndPoint=_rightEndPoint;
	// Guarantee to call CheckAsyncCallOverlappedResult if we call SetUnamangedStructures with a cache in order to
	// avoid a Socket leak in case of error.
	SocketErrorerrorCode=SocketError.SocketError;
	try
	{
	if(_rightEndPoint==null)
	{
	_rightEndPoint=endPointSnapshot;
	}
	errorCode=SocketPal.SendToAsync(_handle,buffer,offset,size,socketFlags,socketAddress,asyncResult);
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SendToAsync returns:{errorCode} size:{size} returning AsyncResult:{asyncResult}");
	}
	catch(ObjectDisposedException)
	{
	_rightEndPoint=oldEndPoint;
	throw;
	}
	// Throw an appropriate SocketException if the native call fails synchronously.
	if(!CheckErrorAndUpdateStatus(errorCode))
	{
	UpdateSendSocketErrorForDisposed(referrorCode);
	// Update the internal state of this socket according to the error before throwing.
	_rightEndPoint=oldEndPoint;

If I'm not missing anything there will be no user callback querying LocalEndpoint in case of failure, however we may end up adding internal code doing so in the future, for example in a tracing call like this:

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

Line 1083 in f8e92ec

if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint}");

Such a call may enforce caching a wrong value for LocalEndpoint permanently, on the other hand it's probably very unlikely we do such a thing by accident. I wonder if it makes sense to add tests to ensure such failing async methods will not alter the sockets internal state in an unwanted way.

@tmds maybe you have some thoughts on this?

[CarnaViire]

@antonfirsov Here is a list of all changes of _rightEndPoint that I have found in the code (sorry for the long list, I would put it under cut if I knew how)

1. setting to new value

   1.1. in constructor
   Socket.cs(165:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(173:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(180:25): _rightEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress));
   Socket.Windows.cs(69:17): _rightEndPoint = ep.Create(socketAddress);

   1.2. after non-blocking connect
   [LocalEndPoint] Socket.cs(319:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [RemoteEndPoint] Socket.cs(361:25): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [Connected] Socket.cs(472:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;

   1.3. if it was null before
   [DoBind] Socket.cs(834:17): _rightEndPoint = endPointSnapshot;
   [SendTo] Socket.cs(1329:17): _rightEndPoint = remoteEP;
   [ReceiveMessageFrom] Socket.cs(1566:21): _rightEndPoint = endPointSnapshot;
   [ReceiveFrom] Socket.cs(1650:21): _rightEndPoint = endPointSnapshot;
   [DoBeginSendTo] Socket.cs(2694:21): _rightEndPoint = endPointSnapshot;
   [BeginReceiveMessageFrom] Socket.cs(3059:21): _rightEndPoint = remoteEP;
   [DoBeginReceiveFrom] Socket.cs(3276:21): _rightEndPoint = endPointSnapshot;
   [ConnectAsync] Socket.cs(3677:21): _rightEndPoint = endPointSnapshot;
   [SendToAsync] Socket.cs(4019:17): _rightEndPoint = endPointSnapshot;
   [DoConnect] Socket.cs(4155:17): _rightEndPoint = endPointSnapshot;
   [BeginConnectEx] Socket.cs(4536:17): _rightEndPoint = endPointSnapshot;

   1.4. on inheriting other socket state
   [UpdateAcceptSocket] Socket.cs(4784:20): socket._rightEndPoint = _rightEndPoint;

2. setting to previous value - on some error

   2.1. on ObjectDisposedException, throwing later
   [DoBeginSendTo] Socket.cs(2703:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3081:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3285:17): _rightEndPoint = oldEndPoint;

   2.2. on any exception, throwing later
   [ConnectAsync] Socket.cs(3700:21): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4547:17): _rightEndPoint = oldEndPoint;

   2.3. on false CheckErrorAndUpdateStatus, throwing later
   [DoBeginSendTo] Socket.cs(2712:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3090:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3294:17): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4563:17): _rightEndPoint = oldEndPoint;

   2.4. on false CheckErrorAndUpdateStatus, NO throwing
   [SendToAsync] Socket.cs(4037:17): _rightEndPoint = oldEndPoint;

3. setting to null - on some error

   3.1. on any exception, throwing later
   [SendToAsync] Socket.cs(4029:17): _rightEndPoint = null;

You suggest to clear _localEndPoint on the reset of _rightEndPoint, which are items 2. and 3. on the list. If we look at subitems of item 1., we can see that most of them are if _rightEndPoint was null before or in the constructor, which means if we clear _localEndPoint at this time we won't actually change anything. And a change of _rightEndPoint after non-blocking connect will only happen once I believe.

What I am trying to say is if we want to clear _localEndPoint on the reset of _rightEndPoint, maybe we could actually return to my original proposal of clearing _localEndPoint on every _rightEndPoint change? The only place where it might possibly unnesessary clear some cached value is this non-blocking connect part (item 1.2.) and we might use _rightEndPoint there instead of new property RightEndPoint specifically in this case.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

So the behavior with wildcard addresses is as follows.

1. If a TCP server socket (listener socket) was bound to a wildcard address, then the accept socket created with accept will be assigned with a specific address based on what interface a client socket connected to. That means we can't copy cached listener's local endpoint if it is wildcard. However, if the listener was bound to a specific address, we can copy that to the accept socket freely.

2. If a TCP client socket was bound to a wildcard address, then it will be assigned with a specific address after connect. That means we should clear the client socket's cached local endpoint if it is wildcard.

3. If an UDP socket was bound to a wildcard address, it stays as wildcard even after sendto and other calls.

I added clearing of cached wildcard local endpoints on connect and accept with clarifying comments. Also added tests that illustrate this behavior.

cc @geoffkizer@antonfirsov

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Outerloop test failures are #40926 and #41381

[antonfirsov]

We need to make sure all independent implementations are covered in the tests, otherwise looks good now.

[antonfirsov]

These 2 tests seem to test the same thing for two different connect implementations. I suggest dedupe their code and also cover the remaining ones: ConnectAsync(SocketAsyncEventArgs) and SocketTaskExtensions.ConnectAsync(this Socket, ....). For this the best would be to utilize SocketTestHelperBase<T>.

[CarnaViire]

Rewrote the tests, please check that I understood you correctly :)

[CarnaViire]

Added _localEndPoint field to cache LocalEndPoint value. Introduced RightEndPoint property to encapsulate clearing _localEndPoint on _rightEndPoint change.Cached value is cleared on connect, error and disconnect. Cached value is cleared on error and disconnect. If the value is a wildcard address, it is also cleared on connect and accept.

Fix#1482

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[karelz]

LGTM, but I let @antonfirsov make the final ok ;)

[antonfirsov]

I'm concerned if we should maybe reset _localEndpoint to null in methods where we also reset _rightEndpoint:

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

Lines 2685 to 2712 in f8e92ec

	EndPoint?oldEndPoint=_rightEndPoint;
	// Guarantee to call CheckAsyncCallOverlappedResult if we call SetUnamangedStructures with a cache in order to
	// avoid a Socket leak in case of error.
	SocketErrorerrorCode=SocketError.SocketError;
	try
	{
	if(_rightEndPoint==null)
	{
	_rightEndPoint=endPointSnapshot;
	}
	errorCode=SocketPal.SendToAsync(_handle,buffer,offset,size,socketFlags,socketAddress,asyncResult);
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SendToAsync returns:{errorCode} size:{size} returning AsyncResult:{asyncResult}");
	}
	catch(ObjectDisposedException)
	{
	_rightEndPoint=oldEndPoint;
	throw;
	}
	// Throw an appropriate SocketException if the native call fails synchronously.
	if(!CheckErrorAndUpdateStatus(errorCode))
	{
	UpdateSendSocketErrorForDisposed(referrorCode);
	// Update the internal state of this socket according to the error before throwing.
	_rightEndPoint=oldEndPoint;

If I'm not missing anything there will be no user callback querying LocalEndpoint in case of failure, however we may end up adding internal code doing so in the future, for example in a tracing call like this:

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

Line 1083 in f8e92ec

if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint}");

Such a call may enforce caching a wrong value for LocalEndpoint permanently, on the other hand it's probably very unlikely we do such a thing by accident. I wonder if it makes sense to add tests to ensure such failing async methods will not alter the sockets internal state in an unwanted way.

@tmds maybe you have some thoughts on this?

[CarnaViire]

@antonfirsov Here is a list of all changes of _rightEndPoint that I have found in the code (sorry for the long list, I would put it under cut if I knew how)

1. setting to new value

   1.1. in constructor
   Socket.cs(165:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(173:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(180:25): _rightEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress));
   Socket.Windows.cs(69:17): _rightEndPoint = ep.Create(socketAddress);

   1.2. after non-blocking connect
   [LocalEndPoint] Socket.cs(319:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [RemoteEndPoint] Socket.cs(361:25): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [Connected] Socket.cs(472:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;

   1.3. if it was null before
   [DoBind] Socket.cs(834:17): _rightEndPoint = endPointSnapshot;
   [SendTo] Socket.cs(1329:17): _rightEndPoint = remoteEP;
   [ReceiveMessageFrom] Socket.cs(1566:21): _rightEndPoint = endPointSnapshot;
   [ReceiveFrom] Socket.cs(1650:21): _rightEndPoint = endPointSnapshot;
   [DoBeginSendTo] Socket.cs(2694:21): _rightEndPoint = endPointSnapshot;
   [BeginReceiveMessageFrom] Socket.cs(3059:21): _rightEndPoint = remoteEP;
   [DoBeginReceiveFrom] Socket.cs(3276:21): _rightEndPoint = endPointSnapshot;
   [ConnectAsync] Socket.cs(3677:21): _rightEndPoint = endPointSnapshot;
   [SendToAsync] Socket.cs(4019:17): _rightEndPoint = endPointSnapshot;
   [DoConnect] Socket.cs(4155:17): _rightEndPoint = endPointSnapshot;
   [BeginConnectEx] Socket.cs(4536:17): _rightEndPoint = endPointSnapshot;

   1.4. on inheriting other socket state
   [UpdateAcceptSocket] Socket.cs(4784:20): socket._rightEndPoint = _rightEndPoint;

2. setting to previous value - on some error

   2.1. on ObjectDisposedException, throwing later
   [DoBeginSendTo] Socket.cs(2703:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3081:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3285:17): _rightEndPoint = oldEndPoint;

   2.2. on any exception, throwing later
   [ConnectAsync] Socket.cs(3700:21): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4547:17): _rightEndPoint = oldEndPoint;

   2.3. on false CheckErrorAndUpdateStatus, throwing later
   [DoBeginSendTo] Socket.cs(2712:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3090:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3294:17): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4563:17): _rightEndPoint = oldEndPoint;

   2.4. on false CheckErrorAndUpdateStatus, NO throwing
   [SendToAsync] Socket.cs(4037:17): _rightEndPoint = oldEndPoint;

3. setting to null - on some error

   3.1. on any exception, throwing later
   [SendToAsync] Socket.cs(4029:17): _rightEndPoint = null;

You suggest to clear _localEndPoint on the reset of _rightEndPoint, which are items 2. and 3. on the list. If we look at subitems of item 1., we can see that most of them are if _rightEndPoint was null before or in the constructor, which means if we clear _localEndPoint at this time we won't actually change anything. And a change of _rightEndPoint after non-blocking connect will only happen once I believe.

What I am trying to say is if we want to clear _localEndPoint on the reset of _rightEndPoint, maybe we could actually return to my original proposal of clearing _localEndPoint on every _rightEndPoint change? The only place where it might possibly unnesessary clear some cached value is this non-blocking connect part (item 1.2.) and we might use _rightEndPoint there instead of new property RightEndPoint specifically in this case.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

So the behavior with wildcard addresses is as follows.

1. If a TCP server socket (listener socket) was bound to a wildcard address, then the accept socket created with accept will be assigned with a specific address based on what interface a client socket connected to. That means we can't copy cached listener's local endpoint if it is wildcard. However, if the listener was bound to a specific address, we can copy that to the accept socket freely.

2. If a TCP client socket was bound to a wildcard address, then it will be assigned with a specific address after connect. That means we should clear the client socket's cached local endpoint if it is wildcard.

3. If an UDP socket was bound to a wildcard address, it stays as wildcard even after sendto and other calls.

I added clearing of cached wildcard local endpoints on connect and accept with clarifying comments. Also added tests that illustrate this behavior.

cc @geoffkizer@antonfirsov

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Outerloop test failures are #40926 and #41381

[antonfirsov]

We need to make sure all independent implementations are covered in the tests, otherwise looks good now.

[antonfirsov]

These 2 tests seem to test the same thing for two different connect implementations. I suggest dedupe their code and also cover the remaining ones: ConnectAsync(SocketAsyncEventArgs) and SocketTaskExtensions.ConnectAsync(this Socket, ....). For this the best would be to utilize SocketTestHelperBase<T>.

[CarnaViire]

Rewrote the tests, please check that I understood you correctly :)

[CarnaViire]

Added _localEndPoint field to cache LocalEndPoint value. Introduced RightEndPoint property to encapsulate clearing _localEndPoint on _rightEndPoint change.Cached value is cleared on connect, error and disconnect. Cached value is cleared on error and disconnect. If the value is a wildcard address, it is also cleared on connect and accept.

Fix#1482

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[karelz]

LGTM, but I let @antonfirsov make the final ok ;)

[antonfirsov]

I'm concerned if we should maybe reset _localEndpoint to null in methods where we also reset _rightEndpoint:

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

Lines 2685 to 2712 in f8e92ec

	EndPoint?oldEndPoint=_rightEndPoint;
	// Guarantee to call CheckAsyncCallOverlappedResult if we call SetUnamangedStructures with a cache in order to
	// avoid a Socket leak in case of error.
	SocketErrorerrorCode=SocketError.SocketError;
	try
	{
	if(_rightEndPoint==null)
	{
	_rightEndPoint=endPointSnapshot;
	}
	errorCode=SocketPal.SendToAsync(_handle,buffer,offset,size,socketFlags,socketAddress,asyncResult);
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SendToAsync returns:{errorCode} size:{size} returning AsyncResult:{asyncResult}");
	}
	catch(ObjectDisposedException)
	{
	_rightEndPoint=oldEndPoint;
	throw;
	}
	// Throw an appropriate SocketException if the native call fails synchronously.
	if(!CheckErrorAndUpdateStatus(errorCode))
	{
	UpdateSendSocketErrorForDisposed(referrorCode);
	// Update the internal state of this socket according to the error before throwing.
	_rightEndPoint=oldEndPoint;

If I'm not missing anything there will be no user callback querying LocalEndpoint in case of failure, however we may end up adding internal code doing so in the future, for example in a tracing call like this:

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

Line 1083 in f8e92ec

if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint}");

Such a call may enforce caching a wrong value for LocalEndpoint permanently, on the other hand it's probably very unlikely we do such a thing by accident. I wonder if it makes sense to add tests to ensure such failing async methods will not alter the sockets internal state in an unwanted way.

@tmds maybe you have some thoughts on this?

[CarnaViire]

@antonfirsov Here is a list of all changes of _rightEndPoint that I have found in the code (sorry for the long list, I would put it under cut if I knew how)

1. setting to new value

   1.1. in constructor
   Socket.cs(165:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(173:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(180:25): _rightEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress));
   Socket.Windows.cs(69:17): _rightEndPoint = ep.Create(socketAddress);

   1.2. after non-blocking connect
   [LocalEndPoint] Socket.cs(319:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [RemoteEndPoint] Socket.cs(361:25): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [Connected] Socket.cs(472:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;

   1.3. if it was null before
   [DoBind] Socket.cs(834:17): _rightEndPoint = endPointSnapshot;
   [SendTo] Socket.cs(1329:17): _rightEndPoint = remoteEP;
   [ReceiveMessageFrom] Socket.cs(1566:21): _rightEndPoint = endPointSnapshot;
   [ReceiveFrom] Socket.cs(1650:21): _rightEndPoint = endPointSnapshot;
   [DoBeginSendTo] Socket.cs(2694:21): _rightEndPoint = endPointSnapshot;
   [BeginReceiveMessageFrom] Socket.cs(3059:21): _rightEndPoint = remoteEP;
   [DoBeginReceiveFrom] Socket.cs(3276:21): _rightEndPoint = endPointSnapshot;
   [ConnectAsync] Socket.cs(3677:21): _rightEndPoint = endPointSnapshot;
   [SendToAsync] Socket.cs(4019:17): _rightEndPoint = endPointSnapshot;
   [DoConnect] Socket.cs(4155:17): _rightEndPoint = endPointSnapshot;
   [BeginConnectEx] Socket.cs(4536:17): _rightEndPoint = endPointSnapshot;

   1.4. on inheriting other socket state
   [UpdateAcceptSocket] Socket.cs(4784:20): socket._rightEndPoint = _rightEndPoint;

2. setting to previous value - on some error

   2.1. on ObjectDisposedException, throwing later
   [DoBeginSendTo] Socket.cs(2703:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3081:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3285:17): _rightEndPoint = oldEndPoint;

   2.2. on any exception, throwing later
   [ConnectAsync] Socket.cs(3700:21): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4547:17): _rightEndPoint = oldEndPoint;

   2.3. on false CheckErrorAndUpdateStatus, throwing later
   [DoBeginSendTo] Socket.cs(2712:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3090:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3294:17): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4563:17): _rightEndPoint = oldEndPoint;

   2.4. on false CheckErrorAndUpdateStatus, NO throwing
   [SendToAsync] Socket.cs(4037:17): _rightEndPoint = oldEndPoint;

3. setting to null - on some error

   3.1. on any exception, throwing later
   [SendToAsync] Socket.cs(4029:17): _rightEndPoint = null;

You suggest to clear _localEndPoint on the reset of _rightEndPoint, which are items 2. and 3. on the list. If we look at subitems of item 1., we can see that most of them are if _rightEndPoint was null before or in the constructor, which means if we clear _localEndPoint at this time we won't actually change anything. And a change of _rightEndPoint after non-blocking connect will only happen once I believe.

What I am trying to say is if we want to clear _localEndPoint on the reset of _rightEndPoint, maybe we could actually return to my original proposal of clearing _localEndPoint on every _rightEndPoint change? The only place where it might possibly unnesessary clear some cached value is this non-blocking connect part (item 1.2.) and we might use _rightEndPoint there instead of new property RightEndPoint specifically in this case.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

So the behavior with wildcard addresses is as follows.

1. If a TCP server socket (listener socket) was bound to a wildcard address, then the accept socket created with accept will be assigned with a specific address based on what interface a client socket connected to. That means we can't copy cached listener's local endpoint if it is wildcard. However, if the listener was bound to a specific address, we can copy that to the accept socket freely.

2. If a TCP client socket was bound to a wildcard address, then it will be assigned with a specific address after connect. That means we should clear the client socket's cached local endpoint if it is wildcard.

3. If an UDP socket was bound to a wildcard address, it stays as wildcard even after sendto and other calls.

I added clearing of cached wildcard local endpoints on connect and accept with clarifying comments. Also added tests that illustrate this behavior.

cc @geoffkizer@antonfirsov

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Outerloop test failures are #40926 and #41381

[antonfirsov]

We need to make sure all independent implementations are covered in the tests, otherwise looks good now.

[antonfirsov]

These 2 tests seem to test the same thing for two different connect implementations. I suggest dedupe their code and also cover the remaining ones: ConnectAsync(SocketAsyncEventArgs) and SocketTaskExtensions.ConnectAsync(this Socket, ....). For this the best would be to utilize SocketTestHelperBase<T>.

[CarnaViire]

Rewrote the tests, please check that I understood you correctly :)

[CarnaViire]

Added _localEndPoint field to cache LocalEndPoint value. Introduced RightEndPoint property to encapsulate clearing _localEndPoint on _rightEndPoint change.Cached value is cleared on connect, error and disconnect. Cached value is cleared on error and disconnect. If the value is a wildcard address, it is also cleared on connect and accept.

Fix#1482

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[karelz]

LGTM, but I let @antonfirsov make the final ok ;)

[antonfirsov]

I'm concerned if we should maybe reset _localEndpoint to null in methods where we also reset _rightEndpoint:

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

Lines 2685 to 2712 in f8e92ec

	EndPoint?oldEndPoint=_rightEndPoint;
	// Guarantee to call CheckAsyncCallOverlappedResult if we call SetUnamangedStructures with a cache in order to
	// avoid a Socket leak in case of error.
	SocketErrorerrorCode=SocketError.SocketError;
	try
	{
	if(_rightEndPoint==null)
	{
	_rightEndPoint=endPointSnapshot;
	}
	errorCode=SocketPal.SendToAsync(_handle,buffer,offset,size,socketFlags,socketAddress,asyncResult);
	if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SendToAsync returns:{errorCode} size:{size} returning AsyncResult:{asyncResult}");
	}
	catch(ObjectDisposedException)
	{
	_rightEndPoint=oldEndPoint;
	throw;
	}
	// Throw an appropriate SocketException if the native call fails synchronously.
	if(!CheckErrorAndUpdateStatus(errorCode))
	{
	UpdateSendSocketErrorForDisposed(referrorCode);
	// Update the internal state of this socket according to the error before throwing.
	_rightEndPoint=oldEndPoint;

If I'm not missing anything there will be no user callback querying LocalEndpoint in case of failure, however we may end up adding internal code doing so in the future, for example in a tracing call like this:

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

Line 1083 in f8e92ec

if(NetEventSource.Log.IsEnabled())NetEventSource.Info(this,$"SRC:{LocalEndPoint}");

Such a call may enforce caching a wrong value for LocalEndpoint permanently, on the other hand it's probably very unlikely we do such a thing by accident. I wonder if it makes sense to add tests to ensure such failing async methods will not alter the sockets internal state in an unwanted way.

@tmds maybe you have some thoughts on this?

[CarnaViire]

@antonfirsov Here is a list of all changes of _rightEndPoint that I have found in the code (sorry for the long list, I would put it under cut if I knew how)

1. setting to new value

   1.1. in constructor
   Socket.cs(165:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(173:25): _rightEndPoint = new IPEndPoint(
   Socket.cs(180:25): _rightEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress));
   Socket.Windows.cs(69:17): _rightEndPoint = ep.Create(socketAddress);

   1.2. after non-blocking connect
   [LocalEndPoint] Socket.cs(319:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [RemoteEndPoint] Socket.cs(361:25): _rightEndPoint = _nonBlockingConnectRightEndPoint;
   [Connected] Socket.cs(472:21): _rightEndPoint = _nonBlockingConnectRightEndPoint;

   1.3. if it was null before
   [DoBind] Socket.cs(834:17): _rightEndPoint = endPointSnapshot;
   [SendTo] Socket.cs(1329:17): _rightEndPoint = remoteEP;
   [ReceiveMessageFrom] Socket.cs(1566:21): _rightEndPoint = endPointSnapshot;
   [ReceiveFrom] Socket.cs(1650:21): _rightEndPoint = endPointSnapshot;
   [DoBeginSendTo] Socket.cs(2694:21): _rightEndPoint = endPointSnapshot;
   [BeginReceiveMessageFrom] Socket.cs(3059:21): _rightEndPoint = remoteEP;
   [DoBeginReceiveFrom] Socket.cs(3276:21): _rightEndPoint = endPointSnapshot;
   [ConnectAsync] Socket.cs(3677:21): _rightEndPoint = endPointSnapshot;
   [SendToAsync] Socket.cs(4019:17): _rightEndPoint = endPointSnapshot;
   [DoConnect] Socket.cs(4155:17): _rightEndPoint = endPointSnapshot;
   [BeginConnectEx] Socket.cs(4536:17): _rightEndPoint = endPointSnapshot;

   1.4. on inheriting other socket state
   [UpdateAcceptSocket] Socket.cs(4784:20): socket._rightEndPoint = _rightEndPoint;

2. setting to previous value - on some error

   2.1. on ObjectDisposedException, throwing later
   [DoBeginSendTo] Socket.cs(2703:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3081:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3285:17): _rightEndPoint = oldEndPoint;

   2.2. on any exception, throwing later
   [ConnectAsync] Socket.cs(3700:21): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4547:17): _rightEndPoint = oldEndPoint;

   2.3. on false CheckErrorAndUpdateStatus, throwing later
   [DoBeginSendTo] Socket.cs(2712:17): _rightEndPoint = oldEndPoint;
   [BeginReceiveMessageFrom] Socket.cs(3090:17): _rightEndPoint = oldEndPoint;
   [DoBeginReceiveFrom] Socket.cs(3294:17): _rightEndPoint = oldEndPoint;
   [BeginConnectEx] Socket.cs(4563:17): _rightEndPoint = oldEndPoint;

   2.4. on false CheckErrorAndUpdateStatus, NO throwing
   [SendToAsync] Socket.cs(4037:17): _rightEndPoint = oldEndPoint;

3. setting to null - on some error

   3.1. on any exception, throwing later
   [SendToAsync] Socket.cs(4029:17): _rightEndPoint = null;

You suggest to clear _localEndPoint on the reset of _rightEndPoint, which are items 2. and 3. on the list. If we look at subitems of item 1., we can see that most of them are if _rightEndPoint was null before or in the constructor, which means if we clear _localEndPoint at this time we won't actually change anything. And a change of _rightEndPoint after non-blocking connect will only happen once I believe.

What I am trying to say is if we want to clear _localEndPoint on the reset of _rightEndPoint, maybe we could actually return to my original proposal of clearing _localEndPoint on every _rightEndPoint change? The only place where it might possibly unnesessary clear some cached value is this non-blocking connect part (item 1.2.) and we might use _rightEndPoint there instead of new property RightEndPoint specifically in this case.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

So the behavior with wildcard addresses is as follows.

1. If a TCP server socket (listener socket) was bound to a wildcard address, then the accept socket created with accept will be assigned with a specific address based on what interface a client socket connected to. That means we can't copy cached listener's local endpoint if it is wildcard. However, if the listener was bound to a specific address, we can copy that to the accept socket freely.

2. If a TCP client socket was bound to a wildcard address, then it will be assigned with a specific address after connect. That means we should clear the client socket's cached local endpoint if it is wildcard.

3. If an UDP socket was bound to a wildcard address, it stays as wildcard even after sendto and other calls.

I added clearing of cached wildcard local endpoints on connect and accept with clarifying comments. Also added tests that illustrate this behavior.

cc @geoffkizer@antonfirsov

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Outerloop test failures are #40926 and #41381

[antonfirsov]

We need to make sure all independent implementations are covered in the tests, otherwise looks good now.

[antonfirsov]

These 2 tests seem to test the same thing for two different connect implementations. I suggest dedupe their code and also cover the remaining ones: ConnectAsync(SocketAsyncEventArgs) and SocketTaskExtensions.ConnectAsync(this Socket, ....). For this the best would be to utilize SocketTestHelperBase<T>.

[CarnaViire]

Rewrote the tests, please check that I understood you correctly :)