Skip to content

Commit a267786

Browse files
KingMobKingMob
authored
Merge pull request #598 from clj-commons/bugfix/redirects-broken-with-method-key
Redirects broken with :method key
2 parents 92b1e36 + a671a21 commit a267786

File tree

5 files changed

+154
-140
lines changed

5 files changed

+154
-140
lines changed

‎project.clj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
(map
2929
#(vector (symbol "io.netty" (str "netty-" %)) netty-version)
3030
netty-modules))
31-
:profiles {:dev {:dependencies [[org.clojure/clojure "1.10.1"]
31+
:profiles {:dev {:dependencies [[org.clojure/clojure "1.10.3"]
3232
[criterium "0.4.6"]
3333
[cheshire "5.10.0"]
3434
[org.slf4j/slf4j-simple "1.7.30"]

‎src/aleph/http.clj

Lines changed: 85 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -276,27 +276,28 @@
276276
by [clj-http](https://github.com/dakrone/clj-http), and returns a deferred representing
277277
the HTTP response. Also allows for a custom `pool` or `middleware` to be defined.
278278
279-
|:---|:---
280-
| `pool` | a custom connection pool
281-
| `middleware` | custom client middleware for the request
282-
| `pool-timeout` | timeout in milliseconds for the pool to generate a connection
283-
| `response-executor` | optional `java.util.concurrent.Executor` that will handle the requests (defaults to a `flow/utilization-executor` of 256 `max-threads` and a `queue-length` of 0)
284-
| `connection-timeout` | timeout in milliseconds for the connection to become established
285-
| `request-timeout` | timeout in milliseconds for the arrival of a response over the established connection
286-
| `read-timeout` | timeout in milliseconds for the response to be completed
287-
| `follow-redirects?` | whether to follow redirects, defaults to `true`; see `aleph.http.client-middleware/handle-redirects`"
279+
Param key | Description
280+
-------------------- | -----------------------------------------------------------------------------------------------------------------------------------------------------------------
281+
`connection-timeout` | timeout in milliseconds for the connection to become established
282+
`follow-redirects?` | whether to follow redirects, defaults to `true`; see `aleph.http.client-middleware/handle-redirects`
283+
`middleware` | custom client middleware for the request
284+
`pool-timeout` | timeout in milliseconds for the pool to generate a connection
285+
`pool` | a custom connection pool
286+
`read-timeout` | timeout in milliseconds for the response to be completed
287+
`request-timeout` | timeout in milliseconds for the arrival of a response over the established connection
288+
`response-executor` | optional `java.util.concurrent.Executor` that will handle the requests (defaults to a `flow/utilization-executor` of 256 `max-threads` and a `queue-length` of 0)"
288289
[{:keys [pool
289290
middleware
290291
pool-timeout
291292
response-executor
292293
connection-timeout
293294
request-timeout
294295
read-timeout]
295-
:or {pool default-connection-pool
296-
response-executor default-response-executor
297-
middleware identity
298-
connection-timeout 6e4} ;; 60 seconds
299-
:as req}]
296+
:or {pool default-connection-pool
297+
response-executor default-response-executor
298+
middleware identity
299+
connection-timeout 6e4} ;; 60 seconds
300+
:as req}]
300301

301302
(executor/with-executor response-executor
302303
((middleware
@@ -306,84 +307,81 @@
306307

307308
;; acquire a connection
308309
(-> (flow/acquire pool k)
309-
(maybe-timeout! pool-timeout)
310-
311-
;; pool timeout triggered
312-
(d/catch' TimeoutException
313-
(fn [^Throwable e]
314-
(d/error-deferred (PoolTimeoutException. e))))
315-
316-
(d/chain'
317-
(fn [conn]
318-
319-
;; get the wrapper for the connection, which may or may not be realized yet
320-
(-> (first conn)
321-
322-
(maybe-timeout! connection-timeout)
323-
324-
;; connection timeout triggered, dispose of the connetion
325-
(d/catch' TimeoutException
326-
(fn [^Throwable e]
327-
(flow/dispose pool k conn)
328-
(d/error-deferred (ConnectionTimeoutException. e))))
329-
330-
;; connection failed, bail out
331-
(d/catch'
332-
(fn [e]
333-
(flow/dispose pool k conn)
334-
(d/error-deferred e)))
335-
336-
;; actually make the request now
337-
(d/chain'
338-
339-
(fn [conn']
340-
341-
(when-not (nil? conn')
342-
(let [end (System/currentTimeMillis)]
343-
(-> (conn' req)
344-
(maybe-timeout! request-timeout)
345-
346-
;; request timeout triggered, dispose of the connection
347-
(d/catch' TimeoutException
348-
(fn [^Throwable e]
349-
(flow/dispose pool k conn)
350-
(d/error-deferred (RequestTimeoutException. e))))
351-
352-
;; request failed, dispose of the connection
353-
(d/catch'
354-
(fn [e]
355-
(flow/dispose pool k conn)
356-
(d/error-deferred e)))
357-
358-
;; clean up the response
359-
(d/chain'
360-
(fn [rsp]
361-
362-
;; only release the connection back once the response is complete
363-
(-> (:aleph/complete rsp)
364-
(maybe-timeout! read-timeout)
365-
310+
(maybe-timeout! pool-timeout)
311+
312+
;; pool timeout triggered
313+
(d/catch' TimeoutException
314+
(fn [^Throwable e]
315+
(d/error-deferred (PoolTimeoutException. e))))
316+
317+
(d/chain'
318+
(fn [conn]
319+
320+
;; get the wrapper for the connection, which may or may not be realized yet
321+
(-> (first conn)
322+
(maybe-timeout! connection-timeout)
323+
324+
;; connection timeout triggered, dispose of the connetion
325+
(d/catch' TimeoutException
326+
(fn [^Throwable e]
327+
(flow/dispose pool k conn)
328+
(d/error-deferred (ConnectionTimeoutException. e))))
329+
330+
;; connection failed, bail out
331+
(d/catch'
332+
(fn [e]
333+
(flow/dispose pool k conn)
334+
(d/error-deferred e)))
335+
336+
;; actually make the request now
337+
(d/chain'
338+
(fn [conn']
339+
(when-not (nil? conn')
340+
(let [end (System/currentTimeMillis)]
341+
(-> (conn' req)
342+
(maybe-timeout! request-timeout)
343+
344+
;; request timeout triggered, dispose of the connection
366345
(d/catch' TimeoutException
367346
(fn [^Throwable e]
368347
(flow/dispose pool k conn)
369-
(d/error-deferred (ReadTimeoutException. e))))
348+
(d/error-deferred (RequestTimeoutException. e))))
349+
350+
;; request failed, dispose of the connection
351+
(d/catch'
352+
(fn [e]
353+
(flow/dispose pool k conn)
354+
(d/error-deferred e)))
370355

356+
;; clean up the connection
371357
(d/chain'
372-
(fn [early?]
373-
(if (or early?
374-
(not (:aleph/keep-alive? rsp))
375-
(<= 400 (:status rsp)))
376-
(flow/dispose pool k conn)
377-
(flow/release pool k conn)))))
378-
(-> rsp
379-
(dissoc :aleph/complete)
380-
(assoc :connection-time (- end start)))))))))
381-
382-
(fn [rsp]
383-
(->> rsp
384-
(middleware/handle-cookies req)
385-
(middleware/handle-redirects request req)))))))))))
386-
req))))
358+
(fn cleanup-conn [rsp]
359+
360+
;; only release the connection back once the response is complete
361+
(-> (:aleph/complete rsp)
362+
(maybe-timeout! read-timeout)
363+
364+
(d/catch' TimeoutException
365+
(fn [^Throwable e]
366+
(flow/dispose pool k conn)
367+
(d/error-deferred (ReadTimeoutException. e))))
368+
369+
(d/chain'
370+
(fn [early?]
371+
(if (or early?
372+
(not (:aleph/keep-alive? rsp))
373+
(<= 400 (:status rsp)))
374+
(flow/dispose pool k conn)
375+
(flow/release pool k conn)))))
376+
(-> rsp
377+
(dissoc :aleph/complete)
378+
(assoc :connection-time (- end start)))))))))
379+
380+
(fn handle-response [rsp]
381+
(->> rsp
382+
(middleware/handle-cookies req)
383+
(middleware/handle-redirects request req)))))))))))
384+
req))))
387385

388386
(defn- req
389387
([method url]

‎src/aleph/http/client_middleware.clj

Lines changed: 40 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -264,45 +264,57 @@
264264
(str "status: " status)
265265
(assoc rsp :body (s/->source body)))))))))))
266266

267+
(defn wrap-method
268+
"Middleware converting the :method option into the :request-method option"
269+
[req]
270+
(if-let [m (:method req)]
271+
(-> req
272+
(dissoc :method)
273+
(assoc :request-method m))
274+
req))
275+
267276
(defn follow-redirect
268277
"Attempts to follow the redirects from the \"location\" header, if no such
269278
header exists (bad server!), returns the response without following the
270279
request."
271280
[client
272281
{:keys [uri url scheme server-name server-port trace-redirects]
273-
:or {trace-redirects []}
274-
:as req}
282+
:or {trace-redirects []}
283+
:as req}
275284
{:keys [body] :as rsp}]
276285
(let [url (or url (str (name scheme) "://" server-name
277-
(when server-port (str ":" server-port)) uri))]
286+
(when server-port (str ":" server-port)) uri))]
278287
(if-let [raw-redirect (get-in rsp [:headers "location"])]
279288
(let [redirect (str (URL. (URL. url) raw-redirect))]
280289
(client
281290
(-> req
282-
(dissoc :query-params)
283-
(assoc :url redirect)
284-
(assoc :trace-redirects (conj trace-redirects redirect)))))
291+
(dissoc :query-params)
292+
(assoc :url redirect)
293+
(assoc :trace-redirects (conj trace-redirects redirect)))))
285294
;; Oh well, we tried, but if no location is set, return the response
286295
rsp)))
287296

288297
(defn handle-redirects
289298
"Middleware that follows redirects in the response. A slingshot exception is
290-
thrown if too many redirects occur. Options
299+
thrown if too many redirects occur. Options:
300+
291301
:follow-redirects - default:true, whether to follow redirects
292302
:max-redirects - default:20, maximum number of redirects to follow
293303
:force-redirects - default:false, force redirecting methods to GET requests
294304
295305
In the response:
296306
:redirects-count - number of redirects
297307
:trace-redirects - vector of sites the request was redirected from"
298-
[client
299-
{:keys [request-method max-redirects redirects-count trace-redirects url]
300-
:or {redirects-count 0
301-
;; max-redirects default taken from Firefox
302-
max-redirects 20}
303-
:as req}
304-
{:keys [status] :as rsp}]
305-
(let [rsp-r (if (empty? trace-redirects)
308+
[client req rsp]
309+
;; The req passed to this fn is the req from BEFORE standard middleware was
310+
;; applied, so we may have to fix :method again
311+
(let [req (wrap-method req)
312+
{:keys [request-method max-redirects redirects-count trace-redirects url]
313+
:or {redirects-count 0
314+
;; max-redirects default taken from Firefox
315+
max-redirects 20}} req
316+
status (:status rsp)
317+
rsp-r (if (empty? trace-redirects)
306318
rsp
307319
(assoc rsp :trace-redirects trace-redirects))]
308320
(cond
@@ -319,35 +331,35 @@
319331

320332
(= 303 status)
321333
(follow-redirect client
322-
(assoc req
323-
:request-method :get
324-
:redirects-count (inc redirects-count))
325-
rsp-r)
334+
(assoc req
335+
:request-method :get
336+
:redirects-count (inc redirects-count))
337+
rsp-r)
326338

327339

328340
(#{301 302} status)
329341
(cond
330342
(#{:get :head} request-method)
331343
(follow-redirect client
332-
(assoc req
333-
:redirects-count (inc redirects-count))
334-
rsp-r)
344+
(assoc req
345+
:redirects-count (inc redirects-count))
346+
rsp-r)
335347

336348
(opt req :force-redirects)
337349
(follow-redirect client
338-
(assoc req
339-
:request-method :get
340-
:redirects-count (inc redirects-count))
341-
rsp-r)
350+
(assoc req
351+
:request-method :get
352+
:redirects-count (inc redirects-count))
353+
rsp-r)
342354

343355
:else
344356
rsp-r)
345357

346358
(#{307 308} status)
347359
(if (or (#{:get :head} request-method)
348-
(opt req :force-redirects))
360+
(opt req :force-redirects))
349361
(follow-redirect client
350-
(assoc req :redirects-count (inc redirects-count)) rsp-r)
362+
(assoc req :redirects-count (inc redirects-count)) rsp-r)
351363
rsp-r)
352364

353365
:else
@@ -489,15 +501,6 @@
489501
(assoc req :basic-auth [user password])
490502
req))
491503

492-
(defn wrap-method
493-
"Middleware converting the :method option into the :request-method option"
494-
[req]
495-
(if-let [m (:method req)]
496-
(-> req
497-
(dissoc :method)
498-
(assoc :request-method m))
499-
req))
500-
501504
(defmulti coerce-form-params
502505
(fn [req] (keyword (content-type-value (:content-type req)))))
503506

‎src/aleph/http/core.clj

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,8 @@
237237
ssl?
238238
ch
239239
(AtomicBoolean. false)
240-
(-> req .uri (.indexOf (int 63))) body)
240+
(-> req .uri (.indexOf (int 63)))
241+
body)
241242
:aleph/request-arrived (System/nanoTime)))
242243

243244
(defn netty-response->ring-response [rsp complete body]

0 commit comments

Comments
 (0)