Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add request retry functionality #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add request retry functionality #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,19 @@ request and returns a response. Convenience wrappers are provided for the http v
- `:http-1.1` prefer HTTP/1.1
- `:http-2` (default) tries to upgrade to HTTP/2, falling back to HTTP/1.1

`retry-handler` Sets an optional retry handler which can be used for retrying requests and implementing more complex
retry strategies such as exponential backoff, retrying on 503s, etc. No retries will be attempted by default. `:auto`
may be supplied which will retry idempotent requests up to 3 times on `IOException` while skipping some other
exceptions such as `InterruptedIOException`, `UnknownHostException`, or `SSLException`.

A retry handler may also be a simple function that receives the following arguments:

response => The response (nil when ex is present)
ex => The exception (nil when response is present)
req => The original request
retry-count => The current retry iteration (starts at 0)

The function should return a boolean or a `CompletableFuture` that resolves to a boolean (useful for backoffs/sleep).

## Usage examples

Expand Down Expand Up @@ -410,13 +423,13 @@ To send a multipart request, `:multipart` may be supplied as a sequence of maps
the `:body` of the request with an `InputStream` of the supplied parts.

```clojure
(hc/get "http://moo.com"
{:multipart [{:name "title" :content "My Awesome Picture"}
{:name "Content/type" :content "image/jpeg"}
{:name "foo.txt" :part-name "eggplant" :content "Eggplants"}
{:name "file" :content (io/file ".test-data")}
{:name "data" :content (.getBytes "hi" "UTF-8") :content-type "text/plain" :file-name "data.txt"}
{:name "jsonParam" :content (io/file ".test-data") :content-type "application/json" :file-name "data.json"}]})
(hc/post "http://moo.com"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making a quick doc change here. Doesn't make sense to do a multipart get request :).

{:multipart [{:name "title" :content "My Awesome Picture"}
{:name "Content/type" :content "image/jpeg"}
{:name "foo.txt" :part-name "eggplant" :content "Eggplants"}
{:name "file" :content (io/file ".test-data")}
{:name "data" :content (.getBytes "hi" "UTF-8") :content-type "text/plain" :file-name "data.txt"}
{:name "jsonParam" :content (io/file ".test-data") :content-type "application/json" :file-name "data.json"}]})
```


Expand Down
163 changes: 113 additions & 50 deletions src/hato/client.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,19 @@
[hato.middleware :as middleware]
[clojure.java.io :as io])
(:import
(java.net.http
HttpClient$Redirect
HttpClient$Version
HttpResponse$BodyHandlers
HttpRequest$BodyPublisher
HttpRequest$BodyPublishers HttpResponse HttpClient HttpRequest HttpClient$Builder HttpRequest$Builder)
(java.net CookiePolicy CookieManager URI ProxySelector Authenticator PasswordAuthentication CookieHandler)
(javax.net.ssl KeyManagerFactory TrustManagerFactory SSLContext)
(java.security KeyStore)
(java.time Duration)
(java.util.function Function Supplier)
(java.io File InputStream)
(clojure.lang ExceptionInfo)))
(java.net.http
HttpClient$Redirect
HttpClient$Version
HttpResponse$BodyHandlers
HttpRequest$BodyPublisher
HttpRequest$BodyPublishers HttpResponse HttpClient HttpRequest HttpClient$Builder HttpRequest$Builder)
(java.net CookiePolicy CookieManager URI ProxySelector Authenticator PasswordAuthentication CookieHandler UnknownHostException)
(javax.net.ssl KeyManagerFactory TrustManagerFactory SSLContext SSLException)
(java.security KeyStore)
(java.time Duration)
(java.util.function Function Supplier BiFunction)
(java.io File InputStream InterruptedIOException IOException)
(java.util.concurrent CompletableFuture CompletionException ExecutionException)))

(defn- ->Authenticator
[v]
Expand All @@ -30,15 +30,6 @@
(getPasswordAuthentication []
(PasswordAuthentication. user (char-array pass))))))))

(defn- ->BodyHandler
"Returns a BodyHandler.

Always returns InputStream that are coerced in middleware.

https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpResponse.BodyHandler.html"
[_]
(HttpResponse$BodyHandlers/ofInputStream))

(defn- ->BodyPublisher
"Returns a BodyPublisher.

Expand Down Expand Up @@ -284,42 +275,114 @@
(->BodyPublisher req))
.build)))

(defn unwrap-async-exception
"Helper function to unwrap common async exceptions to get the root cause such as
an IOException."
[^Exception ex]
(if (or (instance? CompletionException ex)
(instance? ExecutionException ex))
(if-let [cause (.getCause ex)]
(unwrap-async-exception cause)
ex)
ex))

(def retry-exceptions
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think these should be in client.clj or should we move to a separate namespace?

"Common exceptions to retry requests on."
#{IOException})

(def non-retry-exceptions
"Common exceptions to not retry requests on. Borrowed from Apache HttpClient's
`DefaultHttpRequestRetryHandler`."
#{InterruptedIOException UnknownHostException SSLException})

(defn idempotent-request?
"Checks to see if the request is idempotent. It is considered idempotent if the
request method is one of `:get`, `:head`, or `:options`."
[req]
(contains? #{:get :head :options} (:request-method req)))

(defn make-retry-handler
"Constructs a basic retry handler. Options are:

:retry-exceptions => A collection of exceptions required for us to retry the request on.
Defaults to `retry-exceptions`.
:non-retry-exceptions => A collection of exceptions to cause us to skip retrying a specific
request. Defaults to `non-retry-exceptions`.
:max-retry-count => The maximum number of retries before giving up. Defaults to 3."
[{:keys [retry-exceptions non-retry-exceptions max-retry-count]
:or {retry-exceptions retry-exceptions
non-retry-exceptions non-retry-exceptions
max-retry-count 3}}]
(fn [_resp ex req retry-count]
(and (idempotent-request? req)
(some #(instance? % ex) retry-exceptions)
(not (some #(instance? % ex) non-retry-exceptions))
(< retry-count max-retry-count))))

(def default-retry-handler
"A default retry handler which will retry if there is an error with any known retriaretryble
exceptions while skipping non-retry exceptions. By default, this will only retry idempotent
requests (i.e. GET/HEAD/OPTIONS)."
(make-retry-handler {}))

(defn request-with-retries*
"Makes a request retrying on failures as defined by retry-handler"
[http-client http-request body-handler req retry-handler retry-count]
(-> (.sendAsync http-client http-request body-handler)
(.handle
(reify BiFunction
(apply [_ resp e]
[resp (unwrap-async-exception e)])))
(.thenCompose
(reify Function
(apply [_ [resp e]]
(let [retry-fut (if retry-handler
(retry-handler resp e req retry-count)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should put some of this in a map instead so it is more easily extensible in the future? Something like a retry-context map?

(CompletableFuture/completedFuture false))]
(-> (if (instance? CompletableFuture retry-fut)
^CompletableFuture retry-fut
(CompletableFuture/completedFuture retry-fut))
(.thenCompose
(reify Function
(apply [_ retry?]
(if retry?
(request-with-retries* http-client http-request body-handler req retry-handler (inc retry-count))
(if e
(CompletableFuture/failedFuture e)
(CompletableFuture/completedFuture
(response-map {:request req
:http-client http-client
:response resp}))))))))))))))

(defn request*
[{:keys [http-client async? as]
[{:keys [http-client async? retry-handler]
:as req} & [respond raise]]
(let [^HttpClient http-client (if (instance? HttpClient http-client) http-client (build-http-client http-client))
http-request (ring-request->HttpRequest req)
bh (->BodyHandler as)]
(if-not async?
(let [resp (.send http-client http-request bh)]
(response-map
{:request req
:http-client http-client
:response resp}))

(-> (.sendAsync http-client http-request bh)
http-request (ring-request->HttpRequest req)
retry-handler (if (= :auto retry-handler) default-retry-handler retry-handler)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call it

:retry-handler or just :retry?

Should we call it :auto or :default?

Personally, I'd like to make it the retries truly default like Apache HttpClient does, however, I realize that is a decent change to default behavior so opt-in makes sense.

body-handler (HttpResponse$BodyHandlers/ofInputStream)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to bring back the ->BodyHandler function, but confused me at first passing the as field and doing nothing with it.

resp-fut (request-with-retries* http-client http-request body-handler req retry-handler 0)]
(if async?
(-> resp-fut
(.thenApply
(reify Function
(apply [_ resp]
(respond
(response-map
{:request req
:http-client http-client
:response resp})))))
(reify Function
(apply [_ resp-map]
(respond resp-map))))
(.exceptionally
(reify Function
(apply [_ e]
(let [cause (.getCause ^Exception e)]
(if (instance? ExceptionInfo cause)
(raise cause)
(raise e))))))))))
(reify Function
(apply [_ e]
(raise (unwrap-async-exception e))))))
(try @resp-fut
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since .send is still async (it just does a .get internally), I don't think we're losing anything by handling it this way other than we must unwrap ExecutionException

(catch Exception e
(throw (unwrap-async-exception e)))))))

(def default-wrapped-request (middleware/wrap-request request*))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it so every request doesn't have to rebuild the entire middleware chain


(defn request
[req & [respond raise]]
(let [wrapped (middleware/wrap-request request*)]
(if (:async? req)
(wrapped req (or respond identity) (or raise #(throw %)))
(wrapped req))))
(if (:async? req)
(default-wrapped-request req (or respond identity) (or raise #(throw %)))
(default-wrapped-request req)))

(defn- configure-and-execute
"Convenience wrapper"
Expand Down
104 changes: 102 additions & 2 deletions test/hato/client_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
[cheshire.core :as json])
(:import (java.io InputStream)
(java.net ProxySelector CookieHandler CookieManager)
(java.net.http HttpClient$Redirect HttpClient$Version HttpClient)
(java.net.http HttpClient$Redirect HttpClient$Version HttpClient HttpTimeoutException)
(java.time Duration)
(javax.net.ssl SSLContext)
(java.util UUID)))
(java.util UUID)
(java.util.concurrent CompletableFuture)))

(deftest test-build-http-client
(testing "authenticator"
Expand Down Expand Up @@ -289,5 +290,104 @@
(let [r (get "https://nghttp2.org/httpbin/get" {:as :json})]
(is (= :http-2 (:version r))))))

(defn slow-response
[call-count-atom sleep-time-ms]
(swap! call-count-atom inc)
(Thread/sleep sleep-time-ms)
{:status 200
:headers {"Content-Type" "application/json"}
:body (json/generate-string {:sleep sleep-time-ms})})

(defmacro exception-cause
"Evaluates the form, and returns the unwrapped exception."
[form]
`(try ~form
(catch Exception e#
(unwrap-async-exception e#))))

(deftest ^:integration sync-retry-handlers
(testing "Sync non-idempotent requests are not retried"
(let [call-count (atom 0)]
(with-server (fn [_] (slow-response call-count 200))
(is (thrown? HttpTimeoutException (post "http://localhost:1234" {:timeout 100 :retry-handler :auto})))
(is (= 1 @call-count)))))

(testing "Sync requests will make 4 requests total then fail"
(let [call-count (atom 0)]
(with-server (fn [_] (slow-response call-count 200))
(is (thrown? HttpTimeoutException (get "http://localhost:1234" {:timeout 100 :retry-handler :auto})))
(is (= 4 @call-count)))))

(testing "Sync requests will make 2 requests total when second request is a success"
(let [call-count (atom 0)]
(with-server (fn [_] (if (zero? @call-count)
(slow-response call-count 200)
(slow-response call-count 1)))
(is (= 200 (:status (get "http://localhost:1234" {:timeout 100 :retry-handler :auto}))))
(is (= 2 @call-count)))))

(testing "Sync requests honor custom predicate-like retry handlers"
(let [call-count (atom 0)]
(with-server (fn [_] (slow-response call-count 200))
(is (thrown? HttpTimeoutException (get "http://localhost:1234" {:timeout 100
:retry-handler (fn [_resp _ex _req retry-count]
(< retry-count 9))})))
(is (= 10 @call-count)))))

(testing "Sync requests honor custom retry handlers that return a CompletableFuture"
(let [call-count (atom 0)]
(with-server (fn [_] (slow-response call-count 200))
(is (thrown? HttpTimeoutException (get "http://localhost:1234"
{:timeout 100
:retry-handler (fn [_resp _ex _req retry-count]
(CompletableFuture/completedFuture (< retry-count 1)))})))
(is (= 2 @call-count))))))

(deftest ^:integration async-retry-handlers
(testing "Async non-idempotent requests are not retried"
(let [call-count (atom 0)]
(with-server (fn [_] (slow-response call-count 200))
(is (instance? HttpTimeoutException (exception-cause @(post "http://localhost:1234" {:async? true
:timeout 100
:retry-handler :auto}))))
(is (= 1 @call-count)))))

(testing "Async requests will make 4 requests total then fail"
(let [call-count (atom 0)]
(with-server (fn [_] (slow-response call-count 200))
(is (instance? HttpTimeoutException (exception-cause @(get "http://localhost:1234" {:async? true
:timeout 100
:retry-handler :auto}))))
(is (= 4 @call-count)))))

(testing "Async requests will make 2 requests total when second request is a success"
(let [call-count (atom 0)]
(with-server (fn [_] (if (zero? @call-count)
(slow-response call-count 200)
(slow-response call-count 1)))
(is (= 200 (:status @(get "http://localhost:1234" {:async? true
:timeout 100
:retry-handler :auto}))))
(is (= 2 @call-count)))))

(testing "Async requests honor custom predicate-like retry handlers"
(let [call-count (atom 0)]
(with-server (fn [_] (slow-response call-count 200))
(is (instance? HttpTimeoutException (exception-cause @(get "http://localhost:1234"
{:timeout 100
:retry-handler (fn [_resp _ex _req retry-count]
(< retry-count 9))}))))
(is (= 10 @call-count)))))

(testing "Async requests honor custom retry handlers that return a CompletableFuture"
(let [call-count (atom 0)]
(with-server (fn [_] (slow-response call-count 200))
(is (instance? HttpTimeoutException (exception-cause
@(get "http://localhost:1234"
{:timeout 100
:retry-handler (fn [_resp _ex _req retry-count]
(CompletableFuture/completedFuture (< retry-count 1)))}))))
(is (= 2 @call-count))))))

(comment
(run-tests))