Port FullFx fix for SocketConnection sync/async timeout ignore problem. #3994

StephenBonikowsky · 2019-10-24T23:27:39Z

Changeset 1615113 in VSTS.

StephenBonikowsky · 2019-10-24T23:29:50Z

Going into master first then will cherry-pick to release/3.1.0.

@dasetser There is a fair amount of difference between the desktop and core versions of SocketConnection.cs so I had to port the changes manually and not all code paths were present.
Please review.

mconnew · 2019-10-25T04:13:40Z

@StephenBonikowsky, the reason they are different is because I had to modify the implementation because of two reasons.

Socket.Close(int timeout) wasn't originally available in .NET Core so I had to rewrite the close code to use socket lingering capabilities.
Neither System.Net.Sockets or SslStream wasn't available for UWP so I had to rewrite everything on top of the UWP native classes for sockets which didn't have a 1:1 mapping of functionality so I refactored everything to a base class and the concrete implementation.

Neither of this problems exist today so my preference would be to replace the entire SocketConnection class with the implementation from .NET Framework as there are some compromises with the current implementation, e.g. close timeout only works with a granularity of 1 second.

dasetser · 2019-10-25T16:53:47Z

@StephenBonikowsky It looks like you ported the fix correctly to the current implementation. However, I agree with Matt that it would be better if we could port the entire file over again if we can get rid of the compromises in the current implementation.

StephenBonikowsky · 2019-10-28T16:14:57Z

Sounds good, I'll find Matt's commits and see if reverting them makes sense and then porting the SocketConnection class as it is in the full framework.

StephenBonikowsky · 2019-10-29T01:00:42Z

This was not a straight forward port, no copy and paste and just fix formatting issues. There are quite a few things not present in Core that I had to account for. Particularly related to tracing.

Please review this carefully.

src/System.Private.ServiceModel/src/System/ServiceModel/Channels/SocketConnection.cs

mconnew · 2019-10-29T21:17:23Z

Just a couple of small issues around string resources, otherwise LGTM. Fix those SR.Format uses and we can call it good!

src/System.Private.ServiceModel/src/System/ServiceModel/Channels/SocketConnection.cs

StephenBonikowsky · 2019-10-29T23:18:49Z

The new tests added with #4000 are failing because the test server hasn't been updated. Will update it manually and then re-run the checks.

…e problem. * Changeset 1615113 in VSTS.

* There were still many changes that needed to be made to work in Core, too many to name, best just to do a diff. * One big one was the absence of tracing code.

* Reoved SR.Format where not needed. * Added back a comment that was useful. * Use '_socket' instead of the in param 'socket' once '_socket = socket'

mconnew · 2019-10-30T03:51:35Z

…etconnectionfix Port FullFx fix for SocketConnection sync/async timeout ignore problem.

StephenBonikowsky requested review from mconnew, HongGit and dasetser October 24, 2019 23:27

StephenBonikowsky self-assigned this Oct 24, 2019

StephenBonikowsky mentioned this pull request Oct 28, 2019

Calling Open on ICommunication leaves async timer running which closes socket. #3895

Closed

mconnew reviewed Oct 29, 2019

View reviewed changes