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.

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

Restore exception compatibility in TcpListener.EndAccept*** #41745

Merged
merged 4 commits into from
Sep 7, 2020

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Sep 2, 2020

#40476 changed EndAcceptSocket and EndAcceptTcpClient to utilize the Task-based Socket.AcceptAsync implementation instead of the APM code path.
This however introduced a breaking change for the exception being thrown when listener.Stop() has been called, closing the underlying socket between the calls to BeginAccept and EndAccept.
According to the old code and the documentation, we should throw ObjectDisposedException here.

Fixes #41585

/cc @geoffkizer

@ghost
Copy link

ghost commented Sep 2, 2020

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

@antonfirsov antonfirsov added this to the 5.0.0 milestone Sep 2, 2020
{
return TaskToApm.End<TResult>(asyncResult);
}
catch (SocketException ex) when (ex.SocketErrorCode == SocketError.OperationAborted)
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be other cases that cause this exception.

I think we should check explicitly for !_active before changing to ODE. That's what other routines here do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering what are those other cases?

Anyways, I changed the line to catch (SocketException) when (!_active). Want to be equivalent with the old code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering what are those other cases?

Possibly there aren't any. But it seems better to explicitly check _active, rather than just assume this is the only way that exception can happen.

@karelz karelz modified the milestones: 5.0.0, 6.0.0 Sep 2, 2020
@antonfirsov
Copy link
Member Author

antonfirsov commented Sep 3, 2020

@karelz if we push this (and #41585) out to 6.0, we probably need to document a breaking change for 5.0, just to undo it later.

@antonfirsov antonfirsov added the tenet-compatibility Incompatibility with previous versions or .NET Framework label Sep 3, 2020
@antonfirsov
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@karelz
Copy link
Member

karelz commented Sep 3, 2020

@antonfirsov I didn't suggest any pushing out of 5.0 -- I just used correct milestone for the PR - which is in master and hence goes into 6.0. Once we have it merged, we can start porting it to 5.0 RC2 branch.
We had an email discussion that for 5.0 RC1 is ok to just document it as a known problem.

@antonfirsov
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@antonfirsov
Copy link
Member Author

OuterLoop failures are unrelated: #41530, #40798, #41606, #41531, #41381

@antonfirsov antonfirsov merged commit 2fa4e6d into dotnet:master Sep 7, 2020
@antonfirsov
Copy link
Member Author

/backport to release/5.0-rc2

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243045182

antonfirsov pushed a commit that referenced this pull request Sep 8, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TcpListener regression: SocketException at InputShare app
5 participants