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.

Sign up for GitHub

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

Jump to bottom

Throw exceptions in a more consistent way #10353

Merged
merged 8 commits into from Mar 23, 2023
Merged

Throw exceptions in a more consistent way #10353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cae0b4d
Throw exceptions in a more consistent way
molnard Mar 23, 2023
1a39f5c
Do not throw AggregateEx
molnard Mar 23, 2023
0cc30b5
Fix dummy mistake
molnard Mar 23, 2023
230d848
Remove AggregateExceptions
molnard Mar 23, 2023
078d40d
Simplify
molnard Mar 23, 2023
9e7c48a
Remove exception list
molnard Mar 23, 2023
9ea40dd
Fix the test
molnard Mar 23, 2023
0c8e99b
Remove unused method
molnard Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions WalletWasabi.Tests/UnitTests/WabiSabi/Integration/WabiSabiHttpApiIntegrationTests.cs
View file
Open in desktop
@@ -1,4 +1,4 @@
using System.Collections.Immutable;

Check warning on line 1 in WalletWasabi.Tests/UnitTests/WabiSabi/Integration/WabiSabiHttpApiIntegrationTests.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

❌ Getting worse: Large Method 

CoinJoinWithBlameRoundTestAsync increases from 90 to 92 lines of code, threshold = 70
using Microsoft.Extensions.DependencyInjection;
using Moq;
using NBitcoin;
Expand Down Expand Up @@ -398,10 +398,12 @@
var badCoinsTask = Task.Run(async () => await badCoinJoinClient.StartRoundAsync(badCoins, roundState, cts.Token).ConfigureAwait(false), cts.Token);

// BadCoinsTask will throw.
var ex = await Assert.ThrowsAsync<AggregateException>(async () => await Task.WhenAll(new Task[] { badCoinsTask, coinJoinTask }));
Assert.True(ex.InnerExceptions.Last() is TaskCanceledException);
await Task.WhenAll(new Task[] { badCoinsTask, coinJoinTask });
var resultOk = await coinJoinTask;
var resultBad = await badCoinsTask;

Assert.True(coinJoinTask.Result is SuccessfulCoinJoinResult);
Assert.IsType<DisruptedCoinJoinResult>(resultBad);
Assert.IsType<SuccessfulCoinJoinResult>(resultOk);

var broadcastedTx = await transactionCompleted.Task; // wait for the transaction to be broadcasted.
Assert.NotNull(broadcastedTx);
Expand Down
4 changes: 2 additions & 2 deletions WalletWasabi/WabiSabi/Client/AliceClient.cs
View file
Open in desktop
@@ -1,4 +1,4 @@
using NBitcoin;

Check notice on line 1 in WalletWasabi/WabiSabi/Client/AliceClient.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

✅ No longer an issue: Complex Method 

TryToUnregisterAlicesAsync is no longer above the threshold for cyclomatic complexity
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -75,7 +75,7 @@

Logger.LogInfo($"Round ({aliceClient.RoundId}), Alice ({aliceClient.AliceId}): Connection was confirmed.");
}
catch (Exception e) when (e is OperationCanceledException || (e is AggregateException ae && ae.InnerExceptions.Last() is OperationCanceledException))
catch (OperationCanceledException)
{
if (aliceClient is { })
{
Expand Down Expand Up @@ -209,7 +209,7 @@
SmartCoin.CoinJoinInProgress = false;
Logger.LogInfo($"Round ({RoundId}), Alice ({AliceId}): Unregistered {SmartCoin.Outpoint}.");
}
catch (Exception e) when (e is OperationCanceledException || (e is AggregateException ae && ae.InnerExceptions.Last() is OperationCanceledException))
catch (OperationCanceledException e)
{
Logger.LogTrace(e);
}
Expand Down
45 changes: 5 additions & 40 deletions WalletWasabi/WabiSabi/Client/WabiSabiHttpApiClient.cs
View file
Open in desktop
@@ -1,4 +1,4 @@
using Newtonsoft.Json;

Check notice on line 1 in WalletWasabi/WabiSabi/Client/WabiSabiHttpApiClient.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

✅ Getting better: Complex Method 

SendWithRetriesAsync decreases in cyclomatic complexity from 11 to 9, threshold = 9
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
Expand Down Expand Up @@ -61,9 +61,7 @@

private async Task<HttpResponseMessage> SendWithRetriesAsync(RemoteAction action, string jsonString, CancellationToken cancellationToken, TimeSpan? retryTimeout = null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to the PR but retryTimeout naming is questionnable as this variable acts like a requestTimeout

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is cleaner thus we have a request and a retry timeout. retryTimeout is doing something with retries, but the request could be interpreted as the whole request itself with retries or without? Could be confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

attemptTimeout
But I agree it's ultra nit.

{
var exceptions = new Dictionary<Exception, int>();
var start = DateTime.UtcNow;

var totalTimeout = TimeSpan.FromMinutes(30);

using CancellationTokenSource absoluteTimeoutCts = new(totalTimeout);
Expand All @@ -86,10 +84,10 @@

TimeSpan totalTime = DateTime.UtcNow - start;

if (exceptions.Any())
if (attempt > 1)
{
Logger.LogDebug(
$"Received a response for {action} in {totalTime.TotalSeconds:0.##s} after {attempt} failed attempts: {new AggregateException(exceptions.Keys)}.");
$"Received a response for {action} in {totalTime.TotalSeconds:0.##s} after {attempt} failed attempts.");
}
else if (action != RemoteAction.GetStatus)
{
Expand All @@ -101,56 +99,23 @@
catch (HttpRequestException e)
{
Logger.LogTrace($"Attempt {attempt} to perform '{action}' failed with {nameof(HttpRequestException)}: {e.Message}.");
AddException(exceptions, e);
}
catch (OperationCanceledException e)
{
Logger.LogTrace($"Attempt {attempt} to perform '{action}' failed with {nameof(OperationCanceledException)}: {e.Message}.");
AddException(exceptions, e);
}
catch (Exception e)
{
Logger.LogDebug($"Attempt {attempt} to perform '{action}' failed with exception {e}.");

if (exceptions.Any())
{
AddException(exceptions, e);
throw new AggregateException(exceptions.Keys);
}

throw;
}

try
{
// Wait before the next try.
await Task.Delay(250, combinedToken).ConfigureAwait(false);
}
catch (Exception e)
{
AddException(exceptions, e);
}
// Wait before the next try.
await Task.Delay(250, combinedToken).ConfigureAwait(false);

attempt++;
}
while (!combinedToken.IsCancellationRequested);

throw new AggregateException(exceptions.Keys);
}

private static void AddException(Dictionary<Exception, int> exceptions, Exception e)
{
bool Predicate(KeyValuePair<Exception, int> x) => e.GetType() == x.Key.GetType() && e.Message == x.Key.Message;

if (exceptions.Any(Predicate))
{
var first = exceptions.First(Predicate);
exceptions[first.Key]++;
}
else
{
exceptions.Add(e, 1);
}
while (true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was wrong with

while (!combinedToken.IsCancellationRequested);

?

Isn't while (true); a bit dangerous?

We could use the attempt counter to have an absolute maximum tries.
For example 10.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See L114

Copy link
Collaborator Author

@molnard molnard Mar 23, 2023 •
edited

Choose a reason for hiding this comment

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

What was wrong with

The method will just return without throwing an exception.

Isn't while (true); a bit dangerous?

Same risk as before. If combinedToken is never canceled we end up with an infinite loop.

We could use the attempt counter to have an absolute maximum tries.

We do not need to, the max attempts are determined by the timeout.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Ok I took some time to submit my review and I can see you all already talked about it)

}

private async Task<string> SendWithRetriesAsync<TRequest>(RemoteAction action, TRequest request, CancellationToken cancellationToken, TimeSpan? retryTimeout = null) where TRequest : class
Expand Down