SmtpConnection: fix Socket not getting Disposed when LingerState cannot be set.#428
Conversation
There was a problem hiding this comment.
@stephentoub should I change the PR so we can figure out what exact exception gets thrown here? Or do we keep the general catch?
There was a problem hiding this comment.
My preference would be to catch the most specific exception possible, though the risk here is admittedly minimal.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
left a comment
There was a problem hiding this comment.
This change looks fine. How reasonable would it be to write a test that exercises this?
There was a problem hiding this comment.
My preference would be to catch the most specific exception possible, though the risk here is admittedly minimal.
2c2c2b8 to
57df602
Compare
57df602 to
62f67c6
Compare
|
I've updated the PR so instead of hanging, we should see the exception show up in CI now.
Though not a 100% test, the occasional failures in CI mean there is some coverage. |
|
@dotnet/ncl This is good to merge, and will help surface the exception we should catch. |
|
Hello @stephentoub! Because this pull request has the 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 (
|
Fixes https://github.com/dotnet/corefx/issues/40711