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

SmtpClientTest Assertion Fail Fix #76361

Merged
merged 15 commits into from
Oct 24, 2022
Merged

SmtpClientTest Assertion Fail Fix #76361

merged 15 commits into from
Oct 24, 2022

Conversation

liveans
Copy link
Member

@liveans liveans commented Sep 29, 2022 •
edited
Loading

ReplaceHandle is the only function that we're changing the _handle member variable outside of the constructor.
This change worth to try.
Fixes #72830

@liveans liveans added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Sep 29, 2022
@ghost ghost assigned liveans Sep 29, 2022
@ghost
Copy link

ghost commented Sep 29, 2022

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

Issue Details

Trying to collect some information from CI and will try to fix #72830

Author: liveans
Assignees: -
Labels:

NO-MERGE, NO-REVIEW, area-System.Net.Sockets
Milestone: -

@liveans liveans closed this Sep 30, 2022
@liveans liveans reopened this Sep 30, 2022
@liveans liveans closed this Oct 12, 2022
@liveans liveans reopened this Oct 12, 2022
@build-analysis build-analysis bot mentioned this pull request Oct 12, 2022
Closed
2 tasks
@liveans liveans closed this Oct 14, 2022
@liveans liveans reopened this Oct 14, 2022
@build-analysis build-analysis bot mentioned this pull request Oct 16, 2022
Closed
@liveans liveans closed this Oct 16, 2022
@liveans liveans reopened this Oct 16, 2022
@liveans liveans closed this Oct 17, 2022
@liveans liveans reopened this Oct 17, 2022
@liveans liveans removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Oct 18, 2022
@liveans liveans changed the title [IGNORE]: System.Net.Mail Test Failure Fix testing SmtpClientTest Assertion Fail Fix Oct 18, 2022
@liveans liveans marked this pull request as ready for review October 18, 2022 08:46
wfurt
wfurt approved these changes Oct 19, 2022
View reviewed changes
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
antonfirsov reviewed Oct 19, 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.

Would be good to see a repro stressing the race between the multi-connect and the socket disposal. Ideally, we should be able to turn this repro into a test.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs Show resolved Hide resolved
@tmds
Copy link
Member

tmds commented Oct 20, 2022

CI fails look related to the change.

@liveans
Copy link
Member Author

liveans commented Oct 20, 2022

CI fails look related to the change.

Yeah, I'll look at it, today.

@karelz karelz added this to the 8.0.0 milestone Oct 20, 2022
@liveans @antonfirsov
Apply code review suggestion
d51c083 
Co-authored-by: Anton Firszov <[email protected]>
@build-analysis build-analysis bot mentioned this pull request Oct 21, 2022
Closed
@liveans
Copy link
Member Author

liveans commented Oct 24, 2022

I'm merging this, I can create seperate PR for any further review @antonfirsov @wfurt @tmds

@liveans liveans merged commit c092a76 into dotnet:main Oct 24, 2022
@wfurt wfurt mentioned this pull request Nov 8, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Nov 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@antonfirsov antonfirsov antonfirsov left review comments

@tmds tmds tmds left review comments

@wfurt wfurt Awaiting requested review from wfurt

Assignees

@liveans liveans

Labels
area-System.Net.Sockets
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Assertion failure 0 <= fd && fd < sysconf(_SC_OPEN_MAX) in System.Net.Mail.Functional.Tests
5 participants
@liveans @tmds @antonfirsov @wfurt @karelz