Fix #935 retry on IOExceptions too. Refactoring of Helper classes into Chain of responsibility pattern. - #1049
Conversation
…ses into a Chain of Responsibility pattern.
|
@microsoft-github-policy-service agree |
|
Avery-Dunn are you agree to the refactoring of httphelper? |
| private static final int RETRY_DELAY_MS = 1000; | ||
|
|
||
| @Override | ||
| public boolean isRetryable(Exception exception) { |
There was a problem hiding this comment.
The original issue #935 and the commit message mentions retrying on "IOException", but that doesn't seem to be a default reason to retry here.
There was a problem hiding this comment.
Yes, I brainstormed a little bit, and i see much more errors that can come in. Which in fact should be handled in the future.
I mean what should we do, when some HttpClient throws some errors we want to catch?
| * Implementations decide whether a given exception represents a transient failure worth | ||
| * retrying, as opposed to a permanent error (e.g. TLS/certificate misconfiguration). | ||
| */ | ||
| interface IRetryableExceptionPolicy { |
There was a problem hiding this comment.
This interface and implementing classes seem pretty similar to the existing IRetryPolicy, the main difference is taking in an exception instead of an HTTP response.
It might be simpler to just make the IRetryPolicy behavior aware of exceptions.
There was a problem hiding this comment.
This is not correct. IRetryPolicy is handling stuff after a response came in. The IRetryableExceptionPolicy is handling errors on connection level .e. g. (Connection reset for example).
|
|
||
| if (!retryDisabled) { | ||
| if (caughtException != null) { | ||
| if (retryCount < retryableExceptionPolicy.getMaxRetryCount(caughtException) |
There was a problem hiding this comment.
By initializing this class with a DefaultRetryableExceptionPolicy and always using it here alongside the IRetryPolicy, the retry-on-exception behavior is added to every implementation of IRetryPolicy.
ManagedIdentityRetryPolicy/IMDSRetryPolicy have separate, more finely tuned retry behavior which may or may not want to retry on those exceptions.
There was a problem hiding this comment.
I saw the RetryPolicy interface. The problem is more that it is executed on the wrong business logic level.
Means the IRetryPolicy is executed when the request already came back.
We are having here the problem that the request doesnt even came.
Which is a big difference. Therefore we have now to choose A) different interfaces or B) Refactor the IRetryPolicy interface
| * @throws Exception Implementations further down the chain (e.g. the actual send) may throw; implementations | ||
| * that handle/wrap exceptions themselves (e.g. telemetry) do not declare this. | ||
| */ | ||
| IHttpResponse executeHttpRequest(HttpRequest httpRequest, |
There was a problem hiding this comment.
This does not seem like a useful interface: 2/4 implementations (SendRequestChain and RetryRequestChain) don't seem to use the RequestContext and ServiceBundle they're passed, and TelemetryRequestChain hardcodes a null ServiceBundle down the chain.
There was a problem hiding this comment.
Yes some chain implementations doesnt use the filter at all but this is intended to later implement some new "filters" too.
Avery-Dunn
left a comment
Avery-Dunn
left a comment
There was a problem hiding this comment.
The PR description mentions fixing #935, but the vast majority of the changes are dedicated to refactoring HttpHelper.
HttpHelper is about 300 lines of code and has about a dozen internal/private methods focused on different HTTP behaviors, and I don't see how splitting that up into 5 classes with their own entry points, error handling, and inconsistent interface usage improves readability or maintainability.
A targeted fix for #935 would be great (more like #1050) but large refactor for HttpHelper and the new exception policy behavior seem unnecessary and have a lot of side effects and gaps.
The problem I see in this helper method is, that "Everything is in this class". Means this class getting more and more complex over time. Even understanding the inner workings are hard (It really took some time for me). The chain refactoring is just a proposal to make it more reliable in the future but ok, I will open a new PR just for #935. Avery-Dunn I also updated the comments if you are interested. :) |
2 participants
This PR should fix #935 .
I've refactored the HttpHelper classes because it "Does everything like a god object".
Now we have different classes which different responsiblities. e.g. Retry, Telemetry, Throttling etc.
Open for feedback.