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

change win32 to derived types #53146

Conversation

KZedan
Copy link
Contributor

@KZedan KZedan commented May 23, 2021

Fixes #26227

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@AaronRobinsonMSFT
Copy link
Member

/cc @danmoseley

@KZedan
Copy link
Contributor Author

KZedan commented May 23, 2021

FYI seems like NegotiateStreamPal.Windows.cs doesn't like this change which will leave only 2 files eligible for this change. Is this what was expected? Could this be the reason why derived types haven't been used as this may need too much structural change?

It seems like within the System.Net files there are quite a few Win32 exceptions a very few of them fit within the context of these 4:

System.Net.HttpListenerException
System.Net.NetworkInformation.NetworkInformationException
System.Net.Sockets.SocketException
System.Net.WebSockets.WebSocketException

Which are the only available derived types it seems.

@danmoseley
Copy link
Member

If you click through to Azure Devops and look at the raw log, it shows the project files:

2021-05-23T14:39:01.4323284Z D:\workspace\_work\1\s\src\libraries\Common\src\System\Net\Security\NegotiateStreamPal.Windows.cs(172,25): error CS0246: The type or namespace name 'HttpListenerException' could not be found (are you missing a using directive or an assembly reference?) [D:\workspace\_work\1\s\src\libraries\System.Net.Mail\src\System.Net.Mail.csproj]
2021-05-23T14:39:01.4325935Z D:\workspace\_work\1\s\src\libraries\Common\src\System\Net\Security\NegotiateStreamPal.Windows.cs(172,25): error CS0246: The type or namespace name 'HttpListenerException' could not be found (are you missing a using directive or an assembly reference?) [D:\workspace\_work\1\s\src\libraries\System.Net.Security\src\System.Net.Security.csproj]
2021-05-23T14:39:01.4330147Z D:\workspace\_work\1\s\src\libraries\Common\src\System\Net\Security\NegotiateStreamPal.Windows.cs(172,25): error CS0246: The type or namespace name 'HttpListenerException' could not be found (are you missing a using directive or an assembly reference?) [D:\workspace\_work\1\s\src\libraries\System.Net.Http\src\System.Net.Http.csproj]

Basically these are shared source files and 3 of the projects pulling them in don't have a reference to HttpListenerException, which is in System.Net.HttpListener.

I think this is indicating that this is not a good choice. Would SocketException be a better choice? This should be visible to all these libraries. @wfurt is it appropriate?

@danmoseley
Copy link
Member

Oh, I didn't read all of your posting above. OK, I would like to hear @wfurt opinion on whether any of those are appropriate or we need to consider a new type.

@wfurt
Copy link
Member

wfurt commented May 24, 2021

HttpListenerException is certainly not appropriate for SslStream or NegotiateStream. One way to go may be wrapping win32 exception in. AuthenticationException and do the same for low-level OpenSSL codes. With that the exception type would be consistent while preserving the platform details.

@KZedan
Copy link
Contributor Author

KZedan commented May 24, 2021

@danmoseley does what @wfurt suggested require a proposal?

@stephentoub
Copy link
Member

One way to go may be wrapping win32 exception in. AuthenticationException and do the same for low-level OpenSSL codes

AuthenticationException does not derive from Win32Exception, so such a change would change the type thrown to one that could break catch blocks expecting Win32Exception.

@stephentoub
Copy link
Member

Thanks for the PR. As noted by others, though, none of the changes in this PR as it's currently written should be merged... HttpListenerException is not acceptable in these call sites.

@KZedan KZedan closed this May 24, 2021
@KZedan
Copy link
Contributor Author

KZedan commented May 24, 2021

No problems. I noted that only the above 4 exceptions I mentioned are derived from Win32Exception which are not suitable for any of these cases so I have closed the PR.

@KZedan KZedan reopened this May 24, 2021
@KZedan KZedan closed this May 24, 2021
@KZedan KZedan deleted the task/changeWin32ExceptionsToDerivedTypesNetworking branch May 24, 2021 15:20
@ghost ghost locked as resolved and limited conversation to collaborators Jun 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inappropriate use of Win32Exception
5 participants