Skip to content

Conversation

@stephentoub
Copy link
Member

The chain's elements are being disposed of after the chain itself is being disposed of, which means none of the elements are actually being disposed (chain.Dispose nulls out the collection such that accessing the property again lazily initializes it to an empty one).

The chain's elements are being disposed of _after_ the chain itself is being disposed of, which means none of the elements are actually being disposed (chain.Dispose nulls out the collection such that accessing the property again lazily initializes it to an empty one).
@stephentoub stephentoub requested a review from wfurt July 14, 2022 21:06
@ghost ghost assigned stephentoub Jul 14, 2022
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM
thanks.

@wfurt
Copy link
Member

wfurt commented Jul 14, 2022

is there a reason why chain.Dispose() does not dispose the elements as well @bartonjs ?

Also it seems like QUIC does not dispose the chain at all. I'm not sure if that is big deal but we should perhaps put it on TODO list @ManickaP

@stephentoub
Copy link
Member Author

Also it seems like QUIC does not dispose the chain at all

I believe I addressed it in #72221. Is there another case I missed?

@wfurt
Copy link
Member

wfurt commented Jul 14, 2022

no, I look at the source code. #72221 is still on my TODO list to look at.

@bartonjs
Copy link
Member

bartonjs commented Jul 14, 2022

is there a reason why chain.Dispose() does not dispose the elements as well @bartonjs ?

Because this would be surprising:

X509Certificate2 issuerCert;

using (X509Chain chain = new X509Chain())
{
    ...
    chain.Build(...);
    issuerCert = chain.ChainElements[1].Certificate;
}

// ObjectDisposedException?!
Console.WriteLine(issuerCert.Subject);

The caller owns the instances.

@stephentoub stephentoub merged commit d6af976 into dotnet:main Jul 15, 2022
@stephentoub stephentoub deleted the sslhandles branch July 15, 2022 02:39
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2022
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.

3 participants