-
Notifications
You must be signed in to change notification settings - Fork 2
chore: various finish line tasks #23
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
Conversation
{ | ||
if (_speaker != null) | ||
await _speaker.DisposeAsync(); | ||
GC.SuppressFinalize(this); |
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.
I don't understand why this is necessary.
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.
You don't understand why the entire DisposeAsync implementation is necessary or why the GC.SuppressFinalize(this);
is?
If you mean the whole implementation, we're calling it when the app closes to close the named pipe and remove the async receive tasks from the background.
If you mean the GC.SuppressFinalize(this)
thing, it prevents the ~RpcController()
finalizer method from being called. Since we're not using it and we don't need to clean up any unmanaged resources here, we just tell the GC to not bother calling it during a GC run later on.
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.
Yeah, I meant SuppressFinalize
.
I don't see an explicit ~RpcController()
method--I assume this is implicit? Is the need to suppress it required for correctness (i.e. it is not idempotent), or is suppressing it just a performance optimization?
I ask because I'm concerned that this is somewhat fragile. _speaker
is the only resource that needs to be cleaned up today, but if we add a new resource and forget to include it in this method, presumably we'd leak it by supressing the implicit finalizer.
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.
Honestly I don't know that much about how the C# GC works, so I'm not sure. From what I can tell in most cases where you don't explicitly write a finalizer it won't try to run it, so it's probably fine without it.
The main reason this is here is because the linter said we should include it even though we don't have a finalizer:
To prevent derived types with finalizers from having to reimplement IDisposable and to call it, unsealed types without finalizers should still call GC.SuppressFinalize.
So it seems like it's more about convention
Related to #22
Fixes:
coder-vpn.exe
andCoderDesktop.log
=> use registry for locations