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.

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

[SocketException]: New Constructor with string? for SocketException #74744

Merged
merged 8 commits into from
Sep 7, 2022
Merged

[SocketException]: New Constructor with string? for SocketException #74744

merged 8 commits into from
Sep 7, 2022

Conversation

liveans
Copy link
Member

@liveans liveans commented Aug 29, 2022 •
edited
Loading

Closes #69266 and #37150.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 29, 2022
@ghost
Copy link

ghost commented Aug 29, 2022

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

Issue Details

Closes #69266.

Author: liveans
Assignees: -
Labels:

area-System.Net, new-api-needs-documentation
Milestone: -

@stephentoub
Copy link
Member

Thanks, @liveans.

Can you please also add tests?

Also, part of the work for this is removing the ExtendedSocketException type and replacing usage of it with this new constructor (that removal is one of the things motivating the addition of this constructor).

@liveans
Copy link
Member Author

liveans commented Aug 29, 2022

Thanks, @liveans.

Can you please also add tests?

Yes, I'll add the necessary tests!

Also, part of the work for this is removing the ExtendedSocketException type and replacing usage of it with this new constructor (that removal is one of the things motivating the addition of this constructor).

Alright! I'll do this as well.

antonfirsov
antonfirsov reviewed Aug 29, 2022
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can we add some tests and/or try to address #37150 using the new constructor?

Edit: Haven't noticed Stephen's update.

src/libraries/System.Net.Primitives/src/System/Net/SocketException.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Primitives/src/System/Net/SocketException.cs Outdated Show resolved Hide resolved
@liveans
Copy link
Member Author

liveans commented Aug 29, 2022

Can we add some tests and/or try to address #37150 using the new constructor?

Yes! I'll take a look at it.

liveans and others added 2 commits August 29, 2022 16:07
This was referenced Aug 30, 2022
Closed
Open
@liveans liveans removed the community-contribution Indicates that the PR has been added by a community member label Sep 2, 2022
antonfirsov
antonfirsov reviewed Sep 2, 2022
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

A few remarks on tests.

src/libraries/System.Net.Primitives/tests/FunctionalTests/SocketExceptionTest.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Primitives/tests/FunctionalTests/SocketExceptionTest.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Primitives/tests/FunctionalTests/SocketExceptionTest.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Primitives/tests/FunctionalTests/SocketExceptionTest.cs Outdated Show resolved Hide resolved
@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liveans
Copy link
Member Author

liveans commented Sep 5, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub
stephentoub approved these changes Sep 6, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks!

@liveans liveans linked an issue Sep 6, 2022 that may be closed by this pull request
System.Net.Internals.SocketExceptionFactory+ExtendedSocketException #37150
Closed
@liveans
Copy link
Member Author

liveans commented Sep 6, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liveans
Copy link
Member Author

liveans commented Sep 7, 2022

Outerloop failures are #74468

@liveans liveans merged commit e485583 into dotnet:main Sep 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2022
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@antonfirsov antonfirsov antonfirsov approved these changes

Assignees
No one assigned
Labels
area-System.Net new-api-needs-documentation
Projects
None yet
Milestone
8.0.0
4 participants
@liveans @stephentoub @antonfirsov @karelz