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

Fix exception catch block in Dispose_CancelsConnectAsync test #35935

Closed
wants to merge 1 commit into from

Conversation

stephentoub
Copy link
Member

Fixes #35886

@ghost
Copy link

ghost commented May 7, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@davidsh
Copy link
Contributor

davidsh commented May 7, 2020

#35886 was a test failure where the exception was "System.Net.Sockets.SocketException : Bad value for ai_flags".

Is that an "expected" exception side-affect from this test? It almost looks like a programming error.

@davidsh davidsh added this to the 5.0 milestone May 7, 2020
@stephentoub
Copy link
Member Author

Is that an "expected" exception side-affect from this test? It almost looks like a programming error.

This is disposing in the middle of a connect. My assumption is that pretty much any socket exception could occur as a result. Let me know if you think it's actually a concern.

@davidsh
Copy link
Contributor

davidsh commented May 7, 2020

This is disposing in the middle of a connect. My assumption is that pretty much any socket exception could occur as a result. Let me know if you think it's actually a concern.

I do think it is a concern. If it was something like "connection error" etc., that would make more sense with it being a race condition of disposing during connection.

But this exception looks suspect. I think someone more knowledgable about System.Net.Sockets should comment on this.

Basically, I worry that there is some regression in Sockets potentially due to all the recent code churn.

@wfurt @tmds @scalablecory @antonfirsov

@scalablecory
Copy link
Contributor

I think catching all SocketException is too broad and will cover up bugs.

The exception message in the issue points to getaddrinfo misuse or maybe a bad error code being generated somewhere, not to a dispose-related error:

System.Net.Sockets.SocketException : Bad value for ai_flags

I agree with @davidsh this needs a closer look.

@stephentoub
Copy link
Member Author

stephentoub commented May 7, 2020

System.Net.Sockets.SocketException : Bad value for ai_flags

This is what new SocketException((int)SocketError.SocketError) currently produces. SocketError.SocketError is -1, which is the same value as EAI_BADFLAGS on Linux.

@stephentoub
Copy link
Member Author

I think catching all SocketException is too broad and will cover up bugs.

Ok

@stephentoub stephentoub closed this May 7, 2020
@jkotas jkotas deleted the stephentoub-fixcatchblock branch May 7, 2020 18:57
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 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.

Test failure: System.Net.Sockets.Tests.TcpClientTest.Dispose_CancelsConnectAsync
4 participants