RetryingChannel retries IOExceptions rather than all Throwables

[carterkozak]

IOExceptions are thrown by clients when calls fail in unexpected
ways. Other exceptions indicate programming or system errors that
cannot be recovered from.

Before this PR

Burn retries on requests that cannot succeed.

After this PR

==COMMIT_MSG==
RetryingChannel retries IOExceptions rather than all Throwables
==COMMIT_MSG==

Possible downsides?

We may discover other exceptions that we do want to retry that aren't IOExceptions.

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

RetryingChannel retries IOExceptions rather than all Throwables

Check the box to generate changelog(s)

• Generate changelog entry

[ferozco]

👍

[iamdanfox]

SHouldn't we have some more test coverage for this?

[carterkozak]

What do you envision?

[iamdanfox]

I'm worried that we often have these quite specific unit tests that capture behaviour of a single class, but when we combine them all to make our big DialogueChannel we sometimes get unexpected behaviour.

Like for instance the Unable to make a request (queue is full) thing. Is that an intentional change that we're not retrying that anymore?

[carterkozak]

heh, that one was the issue that prompted me to think about this :-) When our queue is full I think it's best to fail fast rather than dropping onto a second time-bounded queue.

Another example is when we send a GET with a body, we throw a SafeIllegalArgumentException which definitely shouldn't be retried.

[iamdanfox]

Ok so I think either some kind of integration test that captures this, or a simulation test case that exercises it would be good.

[carterkozak]

testing was more involved than I anticipated, heh.

[iamdanfox]

Thanks!

[svc-autorelease]

Released 1.18.0