Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 16, 2021

Fix #15740.

cc: @iSazonov

@ghost ghost assigned daxian-dbw Jul 16, 2021
@xtqqczze xtqqczze closed this Jul 16, 2021
@xtqqczze xtqqczze reopened this Jul 16, 2021
@xtqqczze xtqqczze closed this Jul 16, 2021
@xtqqczze xtqqczze reopened this Jul 16, 2021
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM. I made a minor update to reduce the nesting.

@xtqqczze
Copy link
Contributor Author

@daxian-dbw I looked at the implementation again, and removed the unnecessary _disposed bool, as:

  • it is safe to call CancellationTokenSource.Dispose multiple times
  • we don't check that _disposed == false at the beginning of each method (we let CancellationTokenSource throw the ObjectDisposedException instead)

Feel free to revert to reset to 29cdfb4 if you disagree.

@daxian-dbw
Copy link
Member

@xtqqczze In that case, you don't really need to follow the exact IDisposible pattern to have the protected virtual void Dispose(bool disposing) method. This method makes sense only if the type may have derived sub types. You can put CancellationTokenSource.Dispose directly in public void Dispose() and remove GC.SuppressFinalize(this) (calling this method is in case of a sub-type has a finalizer defined. This type doesn't have a finalizer and thus won't be put on the finalizer queue)

@xtqqczze
Copy link
Contributor Author

@xtqqczze In that case, you don't really need to follow the exact IDisposible pattern to have the protected virtual void Dispose(bool disposing) method. This method makes sense only if the type may have derived sub types. You can put CancellationTokenSource.Dispose directly in public void Dispose() and remove GC.SuppressFinalize(this) (calling this method is in case of a sub-type has a finalizer defined. This type doesn't have a finalizer and thus won't be put on the finalizer queue)

@daxian-dbw I already considered those changes, but they are only possible if the class is sealed. Since ConvertToJsonCommand is currently unsealed and a public API I thought it best not to do that.

@xtqqczze
Copy link
Contributor Author

The sealed class issue reminds me of comments in #11820 (review).

@daxian-dbw daxian-dbw merged commit 9ceed48 into PowerShell:master Jul 20, 2021
@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Jul 21, 2021
@iSazonov iSazonov added this to the 7.2.0-preview.8 milestone Jul 21, 2021
@ghost
Copy link

ghost commented Jul 22, 2021

🎉v7.2.0-preview.8 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jul 22, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Leak CancellationTokenSource in ConvertTo-Json

3 participants