-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Improve diagnostics when allocating MsQuicApi #74985
Conversation
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Http/src/System/Net/Http/HttpMethod.cs
Outdated
Show resolved
Hide resolved
a00ca20
to
7cf6eba
Compare
{ | ||
return new MsQuicApi(apiTable); | ||
} | ||
|
||
ThrowHelper.ThrowIfMsQuicError(openStatus); | ||
|
||
// this should unreachable as TryOpenMsQuic returns non-success status on failure | ||
throw new Exception("Failed to create MsQuicApi instance"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you were to invert this condition, you wouldn't need the unreachable throw:
if (!TryLoadMsQuic(out IntPtr msQuicHandle) || !TryOpenMsQuic(msQuicHandle, out QUIC_API_TABLE* apiTable, out openStatus))
{
ThrowHelper.ThrowIfMsQuicError(openStatus);
}
return new MsQuicApi(apiTable);
You could also have a ThrowMsQuicError that was unconditional. Or just use throw GetExceptionForMsQuicStatus(openStatus);
.
Since the change this follows up on was reverted, I am closing this issue, we can pick these changes in #75163 and have them in 1 commit for eventual porting |
This PR improves diagnostics when allocating MsQuicApi instance, this was useful when investigating #74952 and would be nice to get into .NET 7 as part of #74931.