Skip to content

[Code Quality] DapperExecutor.cs — unreachable 'return default' after for-loops with const retry counts #850

@Christophe-Rogiers

Description

@Christophe-Rogiers

Severity: Info

File: src/Servy.Infrastructure/Data/DapperExecutor.cs
Lines: 76 (ExecuteWithRetry), 104 (ExecuteWithRetryAsync)

Description:
Both retry helpers end with an unreachable return default;:

private T? ExecuteWithRetry<T>(Func<T> action)
{
    for (int i = 0; i < SyncMaxRetries; i++)
    {
        try { return action(); }
        catch (SQLiteException ex) when (...)
        {
            if (i == SyncMaxRetries - 1) { ...; throw; }
            ...
        }
    }
    return default;   // unreachable — last iteration either returns or throws
}

SyncMaxRetries and MaxRetries are both private const int = 3, so the loop always runs at least once. The terminal iteration either returns the action's result or rethrows after logging — control never falls out of the loop. The same pattern is present in ExecuteWithRetryAsync at line 104.

The return default is dead code today and only exists to satisfy the compiler's flow analysis that doesn't know the constant is non-zero. Two minor risks:

  1. Future refactors may relax SyncMaxRetries to a parameter or config value (issue [Maintainability] Timing/retry magic numbers scattered across LogTailer, ServiceHelper, DapperExecutor, ProcessHelper, RotatingStreamWriter — consolidate into AppConfig #818 is already pushing toward consolidating timing/retry magic numbers in AppConfig). The day someone passes 0, callers silently get a default(T) (null for reference types, 0 for ints) instead of a proper exception, which is a much worse failure mode than the current rethrow.
  2. Code reviewers reading the method need to mentally prove the line is unreachable, which is friction.

Suggested fix:
Replace the dead line with an explicit throw new InvalidOperationException(\"...\"). That preserves the unreachable-by-construction guarantee for the current const = 3 case, and converts a future 0 into a loud failure instead of a silent default.

throw new InvalidOperationException(\"Retry loop exited without returning or rethrowing — retry count must be > 0.\");

Apply the same change at lines 76 and 104.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions