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

remove unnecessary call #55795

Merged
merged 2 commits into from
Jul 20, 2021
Merged

remove unnecessary call #55795

merged 2 commits into from
Jul 20, 2021

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 16, 2021

We call SafeMsQuicConfigurationHandle.Create twice but used only once configuration.
Since we do not need it past connect I Dispose it there so we don't drag it forward.

Fixes #55158

@wfurt wfurt requested a review from a team July 16, 2021 04:11
@wfurt wfurt self-assigned this Jul 16, 2021
@ghost
Copy link

ghost commented Jul 16, 2021

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

Issue Details

We call SafeMsQuicConfigurationHandle.Create twice but used only once configuration.
Since we do not need it past connect I Dispose it there so we don't drag it forward.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Quic

Milestone: -

@ManickaP
Copy link
Member

Does this closes #55158. Could you link it here and close it with this PR if so?

@wfurt
Copy link
Member Author

wfurt commented Jul 16, 2021

fixes #55158

@karelz
Copy link
Member

karelz commented Jul 16, 2021

@wfurt it needs to be in top post to work with GH -- I added it there ... just FYI

@@ -27,7 +27,7 @@ internal sealed class MsQuicConnection : QuicConnectionProvider

// TODO: remove this.
// This is only used for client-initiated connections, and isn't needed even then once Connect() has been called.
private readonly SafeMsQuicConfigurationHandle? _configuration;
private SafeMsQuicConfigurationHandle? _configuration;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the comment here, I wonder why we don't just create (and dispose) this in ConnectAsync so we don't have to store the value at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we may. That would possibly change where exception can be thrown e.g. in constructor were the options are passed in or in Connect. We would need to store all the options too, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I suppose we need to store the options as well.

Though alternatively, we could pass options to ConnectAsync and never have to store them at all. I don't think we should be making that kind of API change right now, but we can certainly consider it for 7.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wfurt I filed an issue for 7.0 to consider changing the API here to make things simpler, here: #56009

Please take a look at the issue and make sure it looks good to you.

@wfurt wfurt merged commit ef48adf into dotnet:main Jul 20, 2021
@wfurt wfurt deleted the config_55158 branch July 20, 2021 17:20
@karelz karelz added this to the 6.0.0 milestone Jul 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
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.

MsQuic configuration handle is created but not used
5 participants