@wfurt has done an analysis and it seems that this PR has unintentionally affected async Socket I/O. There are cases where it causes RST instead of FIN to be sent. A full write-up will be forthcoming. See analysis #38765.

For now, we are going to revert this PR.

cc: @tmds

/azp run

cc: @krwq @wtgodbe

Would it make sense to iteratively do fixes to that PR instead of reverting? Most of the changes in that PR look good and have rather positive impact on the stability

Given that we're about to ship Preview 7, I agree we should revert. We can try again after Preview 7 if we believe we've really nailed the problems.

fair enough

I went back to the original PR and there are notes abut aborting. So I think that part was at least partially intended.I think we just need to be more careful about compat. It seems like it has potential to cause troubles.

I went back to the original PR and there are notes abut aborting. So I think that part was at least partially intended.I think we just need to be more careful about compat. It seems like it has potential to cause troubles.

Yes, the changes were intentional and should make Linux behave similar to Windows.
@wfurt @davidsh Do we know scenarios where a RST is now being sent where that was not the case with the same code on Windows?

Would it make sense to iteratively do fixes to that PR instead of reverting?

This would have been my preference.

since the aim was to unblock blocking synchronous calls, why does that change Async path as well? That never had issue closing sockets.

The tests run both on the sync and async apis, and assert Windows behavior. To make tests pass, changes were also make to async paths.

I think timing was not good as we saw new test failures on Linux and behavior change on Linux while marching towards preview cutoff on Monday.

The PR end up doing way more than the title claimed or what linked issues referred to.

I did little bit more testing and you are correct that it does match Windows better. I don't know about timing and buffering but it does at least with sending RST.

I think it makes Socket use more difficult requiring more steps to close properly and it will bite Unix developers as this was never needed on Linux (and some applications still may depend on) but it does match our documentation.
https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.close?view=netcore-2.2

Perhaps we should restore conversation about the behavior next week. It seems like this change brings:

• unblock synchronous calls on Dispose (I verified it now works)
• uses SafeHandle instead of IntPtr as general improvement
• tries to match Windows and sends RST on Close without shutdown and/or fiddling with Linger options.

cc: @geoffkizer

We reverted this PR because after it was merged we started seeing test failures across both Linux and Windows, sync and async. We are preparing for Preview 7 and we need to stabilize for that.

The intent of the original PR was to make Linux sync socket behavior behavior especially regarding cancellation. But the PR went beyond that in the end and affected async behaviors and Windows.

If we want to consider these kind of changes in the future, it would be better to have smaller, more defined, PRs.

We are preparing for Preview 7 and we need to stabilize for that.

I understand.

But the PR went beyond that in the end and affected async behaviors and Windows.

This happened based on review comments, and the sum was needed to validate consistent xplat behavior.

@tmds, we just need to do a bit more validation with this which we don't have enough time for until after preview 7.