Skip to content

remove duplicate Connect logic from TcpClient #67951

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
Merged
wfurt merged 2 commits into from
May 17, 2022
Merged

remove duplicate Connect logic from TcpClient #67951

wfurt merged 2 commits into from
May 17, 2022

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Apr 13, 2022

defer the logic to underlying Socket. We already redirect ConnectAsync to Client.ConnectAsync.
+3, -85 lines of code.

fixes #67879

@wfurt wfurt requested review from a team and stephentoub April 13, 2022 06:40
@wfurt wfurt self-assigned this Apr 13, 2022
@ghost
Copy link

ghost commented Apr 13, 2022

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

Issue Details

defer the logic to underlying Socket. We already redirect ConnectAsync to Client.ConnectAsync.
+3, -85 lines of code.

fixes #67879

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Sockets
Milestone: -

ManickaP
ManickaP approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Thanks.

@wfurt
Copy link
Member Author

wfurt commented Apr 13, 2022

Test failures seems related. We should no leak System.Net.Internals.SocketExceptionFactory+ExtendedSocketException public IMHO.

@antonfirsov
Copy link
Contributor

antonfirsov commented Apr 13, 2022 •
edited
Loading

@wfurt there is no way to create a customized (thus more informative) exception message for SocketException without deriving a new type at the moment. See: #37150 (comment).

IMO the tests using Assert.Throws instead of Assert.ThrowsAny are generally wrong unless the exception type is sealed.

Anyways, we should maybe consider a tiny API proposal to add a SocketException(int errorCode, string message) constructor for .NET 7 to get rid of this ugly workaround.

@wfurt wfurt mentioned this pull request May 5, 2022
Closed
@wfurt wfurt merged commit 18410da into dotnet:main May 17, 2022
@wfurt wfurt deleted the connect branch May 17, 2022 16:22
@MihaZupan MihaZupan mentioned this pull request May 18, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ManickaP ManickaP ManickaP approved these changes

@stephentoub stephentoub Awaiting requested review from stephentoub

+1 more reviewer

@Marusyk Marusyk Marusyk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@wfurt wfurt

Labels

area-System.Net.Sockets

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

discrepancy between TcpClient constructor and Connect method

5 participants

@wfurt @antonfirsov @Marusyk @ManickaP @karelz