Skip to content

Do not retry a provider response that carries no data - #56

Open
werwolfby wants to merge 1 commit into
thorstenalpers:mainfrom
werwolfby:fix/fail-fast-on-no-data
Open

Do not retry a provider response that carries no data#56
werwolfby wants to merge 1 commit into
thorstenalpers:mainfrom
werwolfby:fix/fail-fast-on-no-data

Conversation

@werwolfby

Copy link
Copy Markdown

An empty result set is signalled by throwing from inside the retried delegate, and the policy handles every exception. A permanently unknown symbol is therefore retried the full retry count before failing, even though no number of retries can change the outcome.

The clearest symptom is that the cost depends on the company a symbol keeps:

GetQuotesAsync(["AAPL", "BOGUSTICKER", "MSFT"])   OK     95ms -> 2 results: AAPL,MSFT
GetQuotesAsync(["BOGUSTICKER"])                   HANG  >45s
GetRecordsAsync("BOGUSTICKER")                    HANG  >45s
GetQuoteAsync("ENRNQ")  [delisted, but real]      OK     27ms -> ENRNQ

The same unknown symbol costs one request when another symbol in the batch resolves, and retryCount + 1 requests plus the whole back-off budget when it is alone. The time goes entirely into the retry loop, not into talking to the provider.

Cause

src/Services/YahooFinanceService.cs:109, :153, :267 and the equivalents in the other three services signal "no data" by throwing, and src/Utilities/PollyPolicyFactory.cs:13 handles everything:

return Policy
    .Handle<Exception>()          // <-- includes the library's own "no data" signal
    .WaitAndRetryAsync(...)

Changes

  • Add FinanceNetNoDataException for "the provider answered, there is no such data". It derives from FinanceNetException, so existing catch (FinanceNetException) handlers keep working unchanged.

  • Exclude it from the retry predicate, so it fails fast:

    .Handle<Exception>(ex => ex is not FinanceNetNoDataException)
  • Raise it wherever an empty result set was previously reported as a generic failure — the four services and YahooHtmlParser, which is where the Yahoo signals actually originate. The line is drawn at well-formed but empty: malformed responses ("table is null", "data is invalid") and rate limits stay retryable, because for those a retry genuinely can succeed.

  • Let it propagate untouched through the error wrappers, using a filter on the existing catch rather than a second clause, so the original stack trace survives:

    catch (Exception ex) when (ex is not FinanceNetNoDataException)
    {
        throw new FinanceNetException("No records found", ex);
    }
  • Give the no-data throws the context those wrappers used to supply. They mostly carried the bare "All fields empty" constant, which is why wrapping looked necessary; they now name the provider and the request:

    before:  FinanceNetException("No overview found for IBM")
               └── FinanceNetException("All fields empty")
    
    after:   FinanceNetNoDataException("Alpha Vantage returned no overview for IBM")
    

Compatibility

FinanceNetNoDataException derives from FinanceNetException, so catch (FinanceNetException) is unaffected. Two things do change for callers who look closer:

  • Code testing the exact type (ex.GetType() == typeof(FinanceNetException)) will no longer match on no-data paths. This is the point of the change — it is what lets a caller tell an invalid symbol from a temporarily unavailable one.
  • ex.Message on no-data paths changes, and there is no longer an InnerException carrying "All fields empty".

Tests

Adds NoDataFailFastTests, asserting both the exception type and that exactly one HTTP request was made — the retry count is the actual bug, so the request count is the assertion that matters.

Ten existing tests asserted the exact type FinanceNetException on inputs now recognised as no-data, and eight asserted the old wrapper message; they assert the new type and the specific message instead.

TestCategory=Unit passes: 161 tests, 0 failures. No new analyzer warnings.

Note

This is independent of #55 (cancellation), but both touch the same catch blocks in the four services, so whichever merges second will need a rebase — happy to do that. The third part of the cluster, HttpTimeout doubling as the retry back-off base, is on another branch and can follow as a separate PR.


These changes were generated with Claude Code, and I have reviewed them.

🤖 Generated with Claude Code

An empty result set was signalled by throwing from inside the retried
delegate, and the policy handled every exception. A permanently unknown
symbol was therefore retried the full retry count before failing, even
though no number of retries could change the outcome. The same unknown
symbol costs one request when another symbol in the batch resolves, and
retryCount+1 requests plus the whole back-off budget when it is alone.

- Add public FinanceNetNoDataException, derived from FinanceNetException so
  existing catch blocks keep working, for "the provider answered, there is
  no such data".
- Exclude it from the Polly handle predicate, so it fails fast.
- Raise it wherever an empty result set was previously reported as a generic
  failure: the four services and the Yahoo HTML parser. Malformed responses
  ("table is null", "data is invalid", rate limits) stay retryable - only a
  well-formed but empty answer is treated as permanent.
- Let it propagate untouched through the error wrappers, via a filter on the
  existing catch clause, so callers can tell an invalid symbol from a
  transient failure rather than diffing request against response.
- Give the no-data throws the context the wrappers used to supply. They
  mostly carried the bare "All fields empty" constant; they now name the
  provider and the request, e.g. "Alpha Vantage returned no records for IBM".

Adds NoDataFailFastTests, which asserts both the exception type and that
exactly one HTTP request was made. Existing tests that asserted the exact
type FinanceNetException, or the old wrapper message, assert the new type
and the specific message instead.
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

Successfully merging this pull request may close these issues.

1 participant