MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.88k stars 848 forks source link

Cancellable RPC methods #1020

Closed lontivero closed 3 years ago

lontivero commented 3 years ago

Content

This PR adds a cancellation token to most async methods and pass it to the HttpClient.SendAsync method. Depending on the framework's version there are other methods that could also receive the cancellation token but that is out of the scope of this PR, however, the work done here can make that future work easier.

Background

In Wasabi many times we propagate a cancellation token all the way down to all async methods and some times the last call in that chain is an async call to the RPCClient that do not accept a cancellation token.

I assume having an uniform pattern where most methods accept a cancellation token and can cancel a IO task is desirable (not always possible).

A tiny huge problem

For reasons of usability the RPCClient::SendCommandAsync methods receive optional parameters with params object[] what makes these methods really easy to use but that comes with the problem that it is not possible to add a CancellationToken parameter at the end of the list (what is the recommended place to put it) because params must be the latest one.

There is no good solution to that problem and for that reason it is not easy to add CancellationToken to the methods without breaking compatibility so, I decided to DO NOT break it (except for that can be seen in one test). I had to create a new (private) method called InternalSendCommandAsync that is used internally instead of the already published SendCommandAsync. The name InternalSendCommandAsync doesn't sound great, I know.

Update:

This work is needed for https://github.com/zkSNACKs/WalletWasabi/issues/5991

NicolasDorier commented 3 years ago

Adding optional parameter is actually a breaking change if the version of NBitcoin at runtime is different from the one at build time. (this can happen for various reason when there is transitory dependency references, it already happened to me and users of NBitcoin)

I would say, if you really need it, what about adding a CancellationToken to the RPCClient class and use this one in the HttpClient. That's not really clean, I have to admit... or maybe should we bite the bullet and make the breaking changes.

For SendCommandAsync you can just add another overload where the CancellationToken is not the last parameter.

lontivero commented 3 years ago

Yep.

NicolasDorier commented 3 years ago

Reopening this, due to taproot I am doing some breaking changes accross NBitcoin, so let's do this as well.