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

SmtpConnection: fix Socket not getting Disposed when LingerState cannot be set. #428

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

tmds
Copy link
Member

@tmds tmds commented Dec 2, 2019

{
_tcpClient.LingerState = new LingerOption(true, 0);
}
catch
Copy link
Member Author

Choose a reason for hiding this comment

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

@stephentoub should I change the PR so we can figure out what exact exception gets thrown here? Or do we keep the general catch?

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to catch the most specific exception possible, though the risk here is admittedly minimal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the exception something other than derived from System.Exception? Otherwise, why not just write catch (Exception) ?

But I agree it is useful to understand what's going on here. I would expect the exception is probably a SocketException.

Copy link
Member

Choose a reason for hiding this comment

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

should I change the PR so we can figure out what exact exception gets thrown here?

My preference is to catch just the exception types we expect. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

@scalablecory @davidsh @scalablecory I have to change the PR so we know what exception gets thrown here.

I'll revert this try-catch, and also remove the general try-catch in SmtpClient.Abort. That should surface the exception.

We can give it a couple of test runs here. But since the issue doesn't happen often, probably we'll have to merge and wait for some time until it shows up in CI.

@scalablecory scalablecory requested a review from a team December 2, 2019 13:15
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

This change looks fine. How reasonable would it be to write a test that exercises this?

{
_tcpClient.LingerState = new LingerOption(true, 0);
}
catch
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to catch the most specific exception possible, though the risk here is admittedly minimal.

@davidsh davidsh added this to the 5.0 milestone Dec 2, 2019
@tmds
Copy link
Member Author

tmds commented Dec 6, 2019

I've updated the PR so instead of hanging, we should see the exception show up in CI now.
I'm also curious what exception gets thrown here.

How reasonable would it be to write a test that exercises this?

Though not a 100% test, the occasional failures in CI mean there is some coverage.
Maybe we learn something from the exception type on how to make it more reproducible.

@tmds
Copy link
Member Author

tmds commented Dec 17, 2019

@dotnet/ncl This is good to merge, and will help surface the exception we should catch.

@stephentoub stephentoub reopened this Jan 15, 2020
@ghost
Copy link

ghost commented Jan 15, 2020

Hello @stephentoub!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@stephentoub stephentoub merged commit f5d783a into dotnet:master Jan 21, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants