Dispose secret instances

[alex-inftx]

We can dispose Secret instances after use to avoid memory leak. Created Secrets used only for copy their content.

[ghost]

Thanks for your PR, @alex-inftx. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[GrabYourPitchforks]

We can dispose Secret instances after use to avoid memory leak.

FYI there is no memory leak here. Unreferenced instances will be reclaimed by the GC.

[mkArtakMSFT]

I think this is wrong because the key passed in with the returned object will already be disposed when the caller will try to use it.

@halter73 would you agree?

[halter73]

Looking at the implementation of CbcAuthenticatedEncryptor, it copies the buffer and then releases all references to the Secret inside the constructor. So, it's probably safe, but it feels like it's relying on an implementation detail.

@GrabYourPitchforks is right that there is no memory leak, but it does look like it has a SafeHandle field that needs to be finalized if you don't dispose it. It looks like this is generally used only once per key by the singleton KeyRingProvider, so maybe the finalization is okay in order to not rely on the implementation details of the Encryptor constructors.

What's your recommendation @GrabYourPitchforks?

[GrabYourPitchforks]

The general pattern of passing Secret instances around is that the receiver should make a standalone copy (via .ctor(ISecret)) if they need a copy with a standalone lifetime, which allows the caller to maintain control over their own instance's lifetime. So I don't see any issues with this PR from a "this might break in the future" perspective.

The PR seems to be fine from a correctness / regression perspective.

This PR would prevent the finalization of a few objects: let's ballpark it at single digits count per day. The determination of whether this is valuable I'll leave up to the aspnet team. :)

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

[halter73]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[halter73]

Based on @GrabYourPitchforks feedback, I'm going to merge this. If we're not woried about potential regressions due to the encryptor holding onto the Secret instance, we can dispose the Secret to avoid finalization even if it rarely happens.