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

wrap exceptions from callbacks in QuicError.CallbackError #88614

Merged
merged 3 commits into from Jul 17, 2023

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 10, 2023

contributes to #75115.
It uses newly approved QuicError.CallbackError to make it clear what is coming from outside of core Quic.
I was originally thinking about inner try/catch but I decided too keep simple state variable to track if exception needs to be wrapped to keep it simple.

I also added test for scenario described in #75115 e.g. ODE leaking up but not stopping listener.

@wfurt wfurt added this to the 8.0.0 milestone Jul 10, 2023
@wfurt wfurt requested review from ManickaP and a team July 10, 2023 17:45
@wfurt wfurt self-assigned this Jul 10, 2023
@ghost
Copy link

ghost commented Jul 10, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

contributes to #75115.
It uses newly approved QuicError.CallbackError to make it clear what is coming from outside of core Quic.
I was originally thinking about inner try/catch but I decided too keep simple state variable to track if exception needs to be wrapped to keep it simple.

I also added test for scenario described in #75115 e.g. ODE leaking up but not stopping listener.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Quic

Milestone: 8.0.0

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Is there a downside of wrapping the callback calls with extra try-catch block? Or why did you chose to introduce the extra boolean?

@wfurt
Copy link
Member Author

wfurt commented Jul 11, 2023

Is there a downside of wrapping the callback calls with extra try-catch block? Or why did you chose to introduce the extra boolean?

I generally consider trow catch expensive operation. But I don't have any numbers for this particular case. And personally I'm not fan of too much of nesting.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments, thanks.

@wfurt
Copy link
Member Author

wfurt commented Jul 17, 2023

counter test failure looks unrelated.

@wfurt wfurt merged commit bf78b40 into dotnet:main Jul 17, 2023
100 of 103 checks passed
@wfurt wfurt deleted the CallbackError branch July 17, 2023 22:08
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 17, 2023
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.

None yet

2 participants