Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
deansheather merged 4 commits intomainfromdean/finish-line-1
Feb 20, 2025
Merged

Conversation

deansheather
Copy link
Member

Related to#22

Fixes:

  • Exception handler
  • Exit button shouldn't hang the app
  • No default locations forcoder-vpn.exe andCoderDesktop.log => use registry for locations
  • Release build pipeline

{
if (_speaker != null)
await _speaker.DisposeAsync();
GC.SuppressFinalize(this);
Copy link
Collaborator

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.

Copy link
MemberAuthor

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 theGC.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 theGC.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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I meantSuppressFinalize.

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.

Copy link
MemberAuthor

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 thelinter said we should include it even though we don't have a finalizer:

To prevent derived types with finalizers from having to reimplementIDisposable and to call it, unsealed types without finalizers should still callGC.SuppressFinalize.

So it seems like it's more about convention

spikecurtis reacted with thumbs up emoji
@deansheatherdeansheather merged commitacc89ee intomainFeb 20, 2025
3 checks passed
@deansheatherdeansheather deleted the dean/finish-line-1 branchFebruary 20, 2025 05:21
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@spikecurtisspikecurtisspikecurtis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@deansheather@spikecurtis

[8]ページ先頭

©2009-2025 Movatter.jp