Added Option to add Interceptors on Client Level

[fseidl-bauradar]

Description

Added Possibility to add Client wide Interceptors, to check Request and Responses on multiple positions

Purpose

This pull request is a:

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

[fseidl-bauradar]

Previous Pullrequest #2108
Commited Requested Changes

[fseidl-bauradar]

Had to sync with origin, to make this running, hope it's allright, now

[fseidl-bauradar]

My Build are currently failing, because I cannot load Microsoft.CodeAnalysis.CSharp from my nugetSources, therefore I am currently not able to run the tests.
Is there a special source to get the Package from?

Hope this problem only exists on my PC.

[alexeyzimarev]

There's no "special source" as the package is on NuGet.org, which you can check for yourself.

I merged the PR, but I have some follow up work to do. In particular, the interceptors list remains mutable when it's used by the client, and List<T> is not a thread-safe structure, so whilst users can mutate the list of interceptors after the client was instantiated, it can create undesired side effects. I am trying to make the collection immutable when instantiating the client, but it's not going very well, as the read-only options class is generated, and some extra work required to change the property type. The collection can't be held inside RestClient as a private property either as it's used in extension methods.

Another thing is that interceptors are called regardless if the collection is empty or not. Because the interceptor call is awaited, and called unconditionally, there's a performance cost involved even for people who don't use interceptors. I am trying to fix it now.

[alexeyzimarev]

Btw it would be nice to know if making the interceptors collection mutable was your intention, as I see it was used in tests. I am making it immutable now, but we can discuss it if you have a different use case.

[fseidl-bauradar]

When I started the Interceptors, I don't even had Immutable Lists, or Thread Safety in mind, so I just took the default List.
As I would use it you setup the Interceptors once.
And then use it, not manipulating it during running requests.
The only reason why I would change the List after the client is created is the AuthInterceptor, which needs the Client for refreshing the tokens, when they are outdated.
But also this is only setup before the Client is really used.

[fseidl-bauradar]

What exactly, do you mean with the Interceptors are called regardless of it they empty or not?
I you don't have an Interceptor, you enter the function for calling all interceptors, foreach will never be entered, because of no entries to enumerate over.
Therefore the function is directly left again. (Cost 4 Functioncall at max, shouldn't be as heavy)

If you mean the Interceptor Functions such, you defined ValueTasks, according to Brandon Minnick (https://github.com/brminnick/AsyncAwaitBestPractices) this should be better in performance than using Task self.
Additionally if a Function don't call await it should simply behave like a sync function.

[alexeyzimarev]

What exactly, do you mean with the Interceptors are called regardless of it they empty or not?

What I mean is that every async/await call is still creating a state machine, and your OnBeforeXXX were returning Task so no matter if I have interceptors in the request or now, OnBefore and OnAfter were awaited. I basically now allow the interceptors list to be null, so I can quickly escape the interceptors path without entering the state machine. Null checks are cheap compared with async/await and foreach.

Basically, the only work left is to align on handling exceptions. My proposal here is that:

• Interceptors throwing would be treated the same way as legacy hooks
• If ThrowOnXXX is set to true, the client will re-throw
• If it's false (default), the client will return an error result and pass the exception as usual (RestResponse.ErrorException)
• If an exception is thrown by an interceptor before the actual call is made, the client will not execute the call

I believe it allows to cover your case (set the Throw property to true) and also pave the way to get rid of sync hooks in the future.

Alternatively, it is possible to add one more Throw property just for interceptors, but it only makes sense if you want the client to throw when an interceptor throws, but not in case the request fails.

[fseidl-bauradar]

Sounds like a good way

[rassilon]

@alexeyzimarev , public async Task ThrowExceptionIn_InterceptBeforeRequest_ShouldBeCatchableInTest() {
and:
public async Task ThrowExceptionIn_InterceptAfterRequest_ShouldBeCatchableInTest() {
are both failing for me in my rebased redirection branch. Do these fail for you as well?

EDIT: I think this was due to my question I asked in the follow redirect PR. If after request is called inside the redirection loop then the generated exception gets swallowed by exception -> response translation and throw error on failed request option is off for these tests.