Skip to content
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

Possible improvement for Watcher.Dispose() #1591

Open
mikhail-barg opened this issue Oct 2, 2024 · 1 comment
Open

Possible improvement for Watcher.Dispose() #1591

mikhail-barg opened this issue Oct 2, 2024 · 1 comment

Comments

@mikhail-barg
Copy link

Hi!

I think that current implementation of Watcher.Dispose() is a bit lacking:

protected virtual void Dispose(bool disposing)
{
if (!disposedValue)
{
if (disposing)
{
_cts?.Cancel();
_cts?.Dispose();
}
disposedValue = true;
}
}

specifically between _cts?.Cancel() and _cts?.Dispose() calls, we should await for the _watcherLoop task to finish. Otherwise it might attempt to access the Token of already disposed _cts, which is generally undesirable.

Also possible exception would be unobserved, which in turn may be a problem for applications with strict policies on unobserved exceptions.

What do you think? Will you consider a PR like this:

protected virtual void Dispose(bool disposing)
{
    if (!disposedValue)
    {
        if (disposing)
        {
            _cts.Cancel();
            try 
            {
                _watcherLoop.GetAwaiter().GetResult();
            }
            catch 
            {
                //consume possible exception
            }
            _cts.Dispose();
        }

        disposedValue = true;
    }
}
@tg123
Copy link
Member

tg123 commented Oct 3, 2024

i don't think waiting inside dispose is a good idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants