Polly icon indicating copy to clipboard operation
Polly copied to clipboard

Polly.Abstractions, tiny dep for catching polly exceptions in libs?

Open NinoFloris opened this issue 7 years ago • 3 comments

We're currently testing Polly with the (absolutely great) integrations with asp.net 2.1.

One thing we've come across is the following, our service layer gets the HttpClient injected from our asp.net host app and used to just deal with HttpRequestExceptions. However with Polly we have new ones we can't nicely catch like BrokenCircuitException as we don't have the dep on Polly at that level resulting in hacks like:

try 
{
    ...
}
catch (Exception ex) when (
    ex is HttpRequestException ||
    ex.InnerException is HttpRequestException ||
    ex.GetType().BaseType?.FullName.Equals("Polly.ExecutionRejectedException", StringComparison.Ordinal) == true
)

Looking at how the lib is structured I'd really like to see say a Polly.Abstractions we could include at our library level for catching polly exceptions without having to include the full dependency there.

Is this something that would make sense for you guys or am I missing something here?

NinoFloris avatar May 02 '18 17:05 NinoFloris

Thanks @NinoFloris for this great question! (apologies for delayed response). Totally with you about not wanting to capture exceptions by string-matching!

We could split the Polly packaging like this, but I'd like first to understand more about the problems (and possible workrounds), otherwise we could be making a change for a large number of Polly users (split into two pkgs) for just this one use case. (And it takes time to change/administer our build routines but doesn't add new functionality).

The obvious q (and to help me understand more deeply) : Is it an option simply to include the existing Polly package/dll at the layer (service layer?) that needs it? Is the size of the Polly package (at 300KB) a problem for this, or are there other reasons why that may not be suitable?

Community views? Any other reasons why it would be good / not good to split like this?


(Not that we can't change this; but want to understand the real-world impacts more deeply before we embark on changes to packaging.)

reisenberger avatar Sep 08 '18 12:09 reisenberger

See that this request has several 👍 . We may come back to this at a Polly major release, depending on time/availability. It would be suitable for an 'up-for-grabs' if enough of the current unknowns/variables were tied down.

We would ideally want to resolve #281 (eliminate or refine the sync/async split) before pulling out Polly.Abstractions, so that the abstractions contained the definitive set of interfaces we expected to use going forward.

reisenberger avatar Dec 01 '18 10:12 reisenberger

Triage of issues: This remains a good candidate for implementation (core team or up-for-grabs), once v8 is released with the new syntax (those and/or other changes in v8 will affect what gets pulled into the .Abstractions library).

reisenberger avatar Feb 09 '20 15:02 reisenberger

This might be included in Polly v8: https://github.com/App-vNext/Polly/pull/1233#issuecomment-1642074680

martincostello avatar Jul 20 '23 07:07 martincostello

We decided to not create another assembly as Polly.Core is already a small library with no dependencies. Creating something like Polly.Abstractions would introduce possible versioning problems.

For context: https://github.com/App-vNext/Polly/pull/1233#issuecomment-1655063281

@martincostello , is it ok to close this one?

martintmk avatar Aug 29 '23 11:08 martintmk