-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Improve handling of IDisposable instances #18202
Conversation
Moves RunspaceOpening to the ConnectionBase class, so that it has complete control over the instance.
Instead of a customer InternalDispose() method, Dipose(bool disposing) should be overridden. The former method was left in for compatibility, because BaseCommand is public.
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@@ -170,7 +168,10 @@ protected void OnCommandCreationPacketReceived(Guid psGuid) | |||
|
|||
_inProgressCommandsCount++; | |||
|
|||
tracer.WriteMessage("OutOfProcessMediator.OnCommandCreationPacketReceived, in progress command count : " + _inProgressCommandsCount + " psGuid : " + psGuid.ToString()); | |||
using (PowerShellTraceSource tracer = PowerShellTraceSourceFactory.GetTraceSource()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the instance tracer
variable was replaced with this locally scoped variable to match the pattern used in the OnClosePacketReceived
method. A disposable tracer
instance variable would be more efficient, but the enclosing class currently is not IDisposable
, so that would be a breaking change. Additionally, it's derived classes are being used in singleton fashion, and having disposable singletons is probably a discussion topic out of scope for this PR.
@@ -685,7 +685,7 @@ public RunspaceEventQueueItem(RunspaceStateInfo runspaceStateInfo, RunspaceAvail | |||
} | |||
|
|||
// This is to notify once runspace has been opened (RunspaceState.Opened) | |||
internal ManualResetEventSlim RunspaceOpening = new ManualResetEventSlim(false); | |||
private readonly ManualResetEventSlim _runspaceOpening = new ManualResetEventSlim(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made private
, so that ConnectionBase
has full control of the instance, without leaking it's implementation detail to other classes.
} | ||
} | ||
|
||
/// <summary> | ||
/// Do-nothing implementation: derived classes will override as see fit. | ||
/// </summary> | ||
[Obsolete("Derived classes should override Dispose(bool disposing) instead.", false)] | ||
protected virtual void InternalDispose() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the FrontEndCommandBase
class is public, I left this method in for compatibility with potential users. I suppose it could be a good idea to deprecate it now and remove it later.
@hangy The PR is too large and we will wait code review too long 😕 I suggest you split the PR - one fix per PR. |
Done: |
PR Summary
A few previously non-disposed instances are now being disposed correctly when they go out of scope, and a number of methods now use the "standard"
Dispose(bool)
-styleDispose()
-method instead of custom or incomplete calls.PR Context
These changes improve the handling of disposable objects (ie. implementing
IDisposable
) in accordance with the default implementation pattern. Partly addresses the currently disable compiler warnings of the CA2213 and CA2215 type. Partially addresses the violations mentioned in #15803.PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).