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

Do not cancel disposed TokenSource #1419

Merged
merged 6 commits into from May 9, 2019

Conversation

Projects
None yet
2 participants
@lontivero
Copy link
Contributor

commented May 7, 2019

When something wrong happens while processing a wallet (inside WalletService.InitializeAsync) the exception bubbles and disposes the CancelWalletServiceInitialization so, we ends with an unprocessed WalletService but, given it is not null Wasabi displays "There is an already opened wallet".

In other words: when there is an exception while processing the wallet, it doesn't display the correct exception message but the misleading "There is an already opened wallet" error instead.
Related #1413

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

CancelWalletServiceInitialization?.Cancel(); throws ObjectDisposedException. Are you sure about this?

Nitpicking:

  • whitespacing
  • logtrace exception

lontivero added some commits May 7, 2019

@lontivero

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Yes, I am sure. You can verify how it behaves by adding a throw new Exception() after this line:

await WalletService.InitializeAsync(CancelWalletServiceInitialization.Token);

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

Aaah, I got it. I think I met with this many times, but I wasn't sure, because it didn't make sense. Can you write a small software to make sure my theory is correct?

Theory: I think CancellationTokenSource.Dispose(), which is what happens at the end of the using block, does not make the CancellationTokenSource null, so when you call CancellationTokenSource == null then it'll be false, even if it's disposed.

Show resolved Hide resolved WalletWasabi.Gui/Global.cs Outdated
Show resolved Hide resolved WalletWasabi.Gui/Global.cs Outdated
Show resolved Hide resolved WalletWasabi.Gui/Global.cs Outdated
@lontivero

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I think CancellationTokenSource.Dispose(), which is what happens at the end of the using block, does not make the CancellationTokenSource null

Your theory is correct. In fact is not possible to create a method that assigns null to the instance. I mean, it is not possible to do something like instance.MakeMeNullAgain().

Update: when you implement the Disposable pattern, the Dispose() method has to set a local variable disposed = true and in every method check if(disposed) throw new ObjectDisposedException(). Nobody does it but it is the right way to code it and Microsoft implements it right.

nopara73 and others added some commits May 8, 2019

Update WalletWasabi.Gui/Global.cs
Co-Authored-By: lontivero <lontivero@users.noreply.github.com>
Formatting
Co-Authored-By: lontivero <lontivero@users.noreply.github.com>
Add more info to log entry
Co-Authored-By: lontivero <lontivero@users.noreply.github.com>

@nopara73 nopara73 merged commit 96943fc into zkSNACKs:master May 9, 2019

4 checks passed

CodeFactor No issues found.
Details
Wasabi.Linux #20190508.18 succeeded
Details
Wasabi.Osx #20190508.15 succeeded
Details
Wasabi.Windows #20190508.16 succeeded
Details
@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

Update: when you implement the Disposable pattern, the Dispose() method has to set a local variable disposed = true and in every method check if(disposed) throw new ObjectDisposedException(). Nobody does it but it is the right way to code it and Microsoft implements it right.

Can you create an PR with this for this case? Maybe we should start doing this everywhere, because I encountered this issue so many times,

@lontivero lontivero deleted the lontivero:Fixes/Already-Open-Wallet branch Jun 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.