Skip to content

Commit 9800418

Browse files
DerGuteMoritzDerGuteMoritz
committed
Fix HTTP client proxy connection errors and propagation
* Ensure that proxy connection errors have precedence over subsequent errors so that they don't get wrapped in e.g. SSL handshake exceptions. * Fix proxy connection timeout detection (the check wasn't correct anymore). * Propagate ProxyConnectionTimeoutException directly instead of wrapping it in another ConnectionTimeoutException * Add tests for proxy connection errors
1 parent 0b3d548 commit 9800418

File tree

3 files changed

+171
-119
lines changed

3 files changed

+171
-119
lines changed

‎src/aleph/http.clj

+15-3
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
(aleph.utils
2020
ConnectionTimeoutException
2121
PoolTimeoutException
22+
ProxyConnectionTimeoutException
2223
ReadTimeoutException
2324
RequestCancellationException
2425
RequestTimeoutException)
@@ -414,12 +415,23 @@
414415
;; connection establishment failed
415416
(d/catch'
416417
(fn [e]
417-
(if (or (instance? TimeoutException e)
418-
;; Unintuitively, this type doesn't inherit from TimeoutException
419-
(instance? ConnectTimeoutException e))
418+
(cond
419+
;; Handled separately because it inherits from
420+
;; TimeoutException but we don't want to wrap it in
421+
;; ConnectionTimeoutException.
422+
(instance? ProxyConnectionTimeoutException e)
423+
(do
424+
(log/trace "Timed out waiting for proxy connection to be established")
425+
(d/error-deferred e))
426+
427+
(or (instance? TimeoutException e)
428+
;; Unintuitively, this type doesn't inherit from TimeoutException
429+
(instance? ConnectTimeoutException e))
420430
(do
421431
(log/trace "Timed out waiting for connection to be established")
422432
(d/error-deferred (ConnectionTimeoutException. ^Throwable e)))
433+
434+
:else
423435
(do
424436
(log/trace "Connection establishment failed")
425437
(d/error-deferred e)))))

‎src/aleph/http/client.clj

+119-116
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,7 @@
378378
(.setConnectTimeoutMillis ^ProxyHandler handler connection-timeout))
379379
handler))
380380

381-
(defn pending-proxy-connection-handler [early-response-d]
381+
(defn pending-proxy-connection-handler [proxy-connected]
382382
(netty/channel-inbound-handler
383383
:exception-caught
384384
([_ ctx cause]
@@ -388,28 +388,29 @@
388388
headers (when (instance? HttpProxyHandler$HttpProxyConnectException cause)
389389
(.headers ^HttpProxyHandler$HttpProxyConnectException cause))
390390
response (cond
391-
(= "timeout" message)
391+
(.contains message "timeout")
392392
(ProxyConnectionTimeoutException. ^Throwable cause)
393393

394394
(some? headers)
395-
(ex-info message {:headers (http1/headers->map headers)})
395+
(ex-info message {:headers (http1/headers->map headers)} cause)
396396

397397
:else
398398
cause)]
399-
(d/error! early-response-d response)
399+
(d/error! proxy-connected response)
400400
;; client handler should take care of the rest
401401
(netty/close ctx))))
402402

403403
:user-event-triggered
404404
([this ctx evt]
405405
(when (instance? ProxyConnectionEvent evt)
406-
(.remove (.pipeline ctx) this))
406+
(.remove (.pipeline ctx) this)
407+
(d/success! proxy-connected true))
407408
(.fireUserEventTriggered ^ChannelHandlerContext ctx evt))))
408409

409410
(defn- add-proxy-handlers
410411
"Inserts handlers for proxying through a server"
411-
[^ChannelPipeline p early-response-d proxy-options ssl?]
412-
(when (some? proxy-options)
412+
[^ChannelPipeline p proxy-connected proxy-options ssl?]
413+
(if (some? proxy-options)
413414
(let [proxy (proxy-handler (assoc proxy-options :ssl? ssl?))]
414415
(.addFirst p "proxy" ^ChannelHandler proxy)
415416
;; well, we need to wait before the proxy responded with
@@ -420,7 +421,8 @@
420421
"proxy"
421422
"pending-proxy-connection"
422423
^ChannelHandler
423-
(pending-proxy-connection-handler early-response-d)))))
424+
(pending-proxy-connection-handler proxy-connected))))
425+
(d/success! proxy-connected false))
424426
p)
425427

426428
(defn- setup-http1-pipeline
@@ -472,10 +474,10 @@
472474
473475
Can't use an ApnHandler/ApplicationProtocolNegotiationHandler here,
474476
because it's tricky to run Manifold code on Netty threads."
475-
[{:keys [ssl? remote-address ssl-context ssl-endpoint-id-alg proxy-options early-response-d]}]
477+
[{:keys [ssl? remote-address ssl-context ssl-endpoint-id-alg proxy-options proxy-connected]}]
476478
(fn pipeline-builder*
477479
[^ChannelPipeline pipeline]
478-
(add-proxy-handlers pipeline early-response-d proxy-options ssl?)
480+
(add-proxy-handlers pipeline proxy-connected proxy-options ssl?)
479481
(when ssl?
480482
(do
481483
(.addLast pipeline
@@ -803,10 +805,10 @@
803805
(some? log-activity) (netty/activity-logger "aleph-client" log-activity)
804806
:else nil)
805807

806-
early-response-d (d/deferred)
808+
proxy-connected (d/deferred)
807809
pipeline-builder (make-pipeline-builder
808810
(assoc opts
809-
:early-response-d early-response-d
811+
:proxy-connected proxy-connected
810812
:ssl? ssl?
811813
:ssl-context ssl-context
812814
:ssl-endpoint-id-alg ssl-endpoint-id-alg
@@ -827,110 +829,111 @@
827829

828830
(attach-on-close-handler ch-d on-closed)
829831

830-
(d/alt'
831-
early-response-d
832-
(d/chain' ch-d
833-
(fn setup-client
834-
[^Channel ch]
835-
(log/debug "Channel:" ch)
836-
837-
;; We know the SSL handshake must be complete because create-client wraps the
838-
;; future with maybe-ssl-handshake-future, so we can get the negotiated
839-
;; protocol, falling back to HTTP/1.1 by default.
840-
(let [pipeline (.pipeline ch)
841-
protocol (cond
842-
ssl?
843-
(or (-> pipeline
844-
^SslHandler (.get ^Class SslHandler)
845-
(.applicationProtocol))
846-
ApplicationProtocolNames/HTTP_1_1) ; Not using ALPN, HTTP/2 isn't allowed
847-
848-
force-h2c?
849-
(do
850-
(log/info "Forcing HTTP/2 over cleartext. Be sure to do this only with servers you control.")
851-
ApplicationProtocolNames/HTTP_2)
852-
853-
:else
854-
ApplicationProtocolNames/HTTP_1_1) ; Not using SSL, HTTP/2 isn't allowed unless h2c requested
855-
setup-opts (assoc opts
856-
:authority authority
857-
:ch ch
858-
:server? false
859-
:keep-alive? keep-alive?
860-
:keep-alive?' keep-alive?'
861-
:logger logger
862-
:non-tun-proxy? non-tun-proxy?
863-
:pipeline pipeline
864-
:pipeline-transform pipeline-transform
865-
:raw-stream? raw-stream?
866-
:remote-address remote-address
867-
:response-buffer-size response-buffer-size
868-
:ssl-context ssl-context
869-
:ssl? ssl?)]
870-
871-
(log/debug (str "Using HTTP protocol: " protocol)
872-
{:authority authority
873-
:ssl? ssl?
874-
:force-h2c? force-h2c?})
875-
876-
;; can't use ApnHandler, because we need to coordinate with Manifold code
877-
(let [http-req-handler
878-
(cond (.equals ApplicationProtocolNames/HTTP_1_1 protocol)
879-
(setup-http1-client setup-opts)
880-
881-
(.equals ApplicationProtocolNames/HTTP_2 protocol)
882-
(do
883-
(http2/setup-conn-pipeline setup-opts)
884-
(http2-req-handler setup-opts))
885-
886-
:else
887-
(do
888-
(let [msg (str "Unknown protocol: " protocol)
889-
e (IllegalStateException. msg)]
890-
(log/error e msg)
891-
(netty/close ch)
892-
(throw e))))]
893-
894-
;; Both Netty and Aleph are set up, unpause the pipeline
895-
(when (.get pipeline "pause-handler")
896-
(log/debug "Unpausing pipeline")
897-
(.remove pipeline "pause-handler"))
898-
899-
(fn http-req-fn
900-
[req]
901-
(log/trace "http-req-fn fired")
902-
(log/debug "client request:" (pr-str req))
903-
904-
;; If :aleph/close is set in the req, closes the channel and
905-
;; returns a deferred containing the result.
906-
(if (or (contains? req :aleph/close)
907-
(contains? req ::close))
908-
(-> ch (netty/close) (netty/wrap-future))
909-
910-
(let [t0 (System/nanoTime)
911-
;; I suspect the below is an error for http1
912-
;; since the shared handler might not match.
913-
;; Should work for HTTP2, though
914-
raw-stream? (get req :raw-stream? raw-stream?)]
915-
916-
(if (or (not (.isActive ch))
917-
(not (.isOpen ch)))
918-
919-
(d/error-deferred
920-
(ex-info "Channel is inactive/closed."
921-
{:req req
922-
:ch ch
923-
:open? (.isOpen ch)
924-
:active? (.isActive ch)}))
925-
926-
(-> (http-req-handler req)
927-
(d/chain' (rsp-handler
928-
{:ch ch
929-
:keep-alive? keep-alive? ; why not keep-alive?'
930-
:raw-stream? raw-stream?
931-
:req req
932-
:response-buffer-size response-buffer-size
933-
:t0 t0}))))))))))))))
832+
(d/chain'
833+
proxy-connected
834+
(fn [_]
835+
(d/chain' ch-d
836+
(fn setup-client
837+
[^Channel ch]
838+
(log/debug "Channel:" ch)
839+
840+
;; We know the SSL handshake must be complete because create-client wraps the
841+
;; future with maybe-ssl-handshake-future, so we can get the negotiated
842+
;; protocol, falling back to HTTP/1.1 by default.
843+
(let [pipeline (.pipeline ch)
844+
protocol (cond
845+
ssl?
846+
(or (-> pipeline
847+
^SslHandler (.get ^Class SslHandler)
848+
(.applicationProtocol))
849+
ApplicationProtocolNames/HTTP_1_1) ; Not using ALPN, HTTP/2 isn't allowed
850+
851+
force-h2c?
852+
(do
853+
(log/info "Forcing HTTP/2 over cleartext. Be sure to do this only with servers you control.")
854+
ApplicationProtocolNames/HTTP_2)
855+
856+
:else
857+
ApplicationProtocolNames/HTTP_1_1) ; Not using SSL, HTTP/2 isn't allowed unless h2c requested
858+
setup-opts (assoc opts
859+
:authority authority
860+
:ch ch
861+
:server? false
862+
:keep-alive? keep-alive?
863+
:keep-alive?' keep-alive?'
864+
:logger logger
865+
:non-tun-proxy? non-tun-proxy?
866+
:pipeline pipeline
867+
:pipeline-transform pipeline-transform
868+
:raw-stream? raw-stream?
869+
:remote-address remote-address
870+
:response-buffer-size response-buffer-size
871+
:ssl-context ssl-context
872+
:ssl? ssl?)]
873+
874+
(log/debug (str "Using HTTP protocol: " protocol)
875+
{:authority authority
876+
:ssl? ssl?
877+
:force-h2c? force-h2c?})
878+
879+
;; can't use ApnHandler, because we need to coordinate with Manifold code
880+
(let [http-req-handler
881+
(cond (.equals ApplicationProtocolNames/HTTP_1_1 protocol)
882+
(setup-http1-client setup-opts)
883+
884+
(.equals ApplicationProtocolNames/HTTP_2 protocol)
885+
(do
886+
(http2/setup-conn-pipeline setup-opts)
887+
(http2-req-handler setup-opts))
888+
889+
:else
890+
(do
891+
(let [msg (str "Unknown protocol: " protocol)
892+
e (IllegalStateException. msg)]
893+
(log/error e msg)
894+
(netty/close ch)
895+
(throw e))))]
896+
897+
;; Both Netty and Aleph are set up, unpause the pipeline
898+
(when (.get pipeline "pause-handler")
899+
(log/debug "Unpausing pipeline")
900+
(.remove pipeline "pause-handler"))
901+
902+
(fn http-req-fn
903+
[req]
904+
(log/trace "http-req-fn fired")
905+
(log/debug "client request:" (pr-str req))
906+
907+
;; If :aleph/close is set in the req, closes the channel and
908+
;; returns a deferred containing the result.
909+
(if (or (contains? req :aleph/close)
910+
(contains? req ::close))
911+
(-> ch (netty/close) (netty/wrap-future))
912+
913+
(let [t0 (System/nanoTime)
914+
;; I suspect the below is an error for http1
915+
;; since the shared handler might not match.
916+
;; Should work for HTTP2, though
917+
raw-stream? (get req :raw-stream? raw-stream?)]
918+
919+
(if (or (not (.isActive ch))
920+
(not (.isOpen ch)))
921+
922+
(d/error-deferred
923+
(ex-info "Channel is inactive/closed."
924+
{:req req
925+
:ch ch
926+
:open? (.isOpen ch)
927+
:active? (.isActive ch)}))
928+
929+
(-> (http-req-handler req)
930+
(d/chain' (rsp-handler
931+
{:ch ch
932+
:keep-alive? keep-alive? ; why not keep-alive?'
933+
:raw-stream? raw-stream?
934+
:req req
935+
:response-buffer-size response-buffer-size
936+
:t0 t0})))))))))))))))
934937

935938

936939

‎test/aleph/http_test.clj

+37
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
(:import
1919
(aleph.utils
2020
ConnectionTimeoutException
21+
ProxyConnectionTimeoutException
2122
RequestCancellationException
2223
RequestTimeoutException)
2324
(clojure.lang
@@ -42,6 +43,7 @@
4243
HttpRequestDecoder
4344
LastHttpContent)
4445
(io.netty.handler.codec.http2 Http2HeadersFrame)
46+
(io.netty.handler.proxy ProxyConnectException)
4547
(java.io
4648
Closeable
4749
File)
@@ -1710,6 +1712,41 @@
17101712
(select-keys @(d/timeout! proxy-request 1000)
17111713
[:request-method :uri :headers :body]))))))))
17121714

1715+
(deftest test-client-proxy-connection-timeout
1716+
(with-http-ssl-servers basic-handler {}
1717+
(let [proxy-server (tcp/start-server (fn [_ _]
1718+
;; Accept connection and let it linger so that the
1719+
;; client's proxy connection timeout fires.
1720+
)
1721+
{:port 0
1722+
:shutdown-timeout 0})]
1723+
(binding [*connection-options* {:proxy-options {:host "localhost"
1724+
:port (netty/port proxy-server)
1725+
:connection-timeout 10}}]
1726+
(with-server proxy-server
1727+
(try
1728+
@(d/timeout! (http-get "/string") 1000)
1729+
(is (= true false) "Should have thrown an exception")
1730+
(catch Exception e
1731+
(is (instance? ProxyConnectionTimeoutException e)))))))))
1732+
1733+
(deftest test-client-proxy-connect-error
1734+
(with-http-ssl-servers basic-handler {}
1735+
(let [proxy-server (tcp/start-server (fn [stream _]
1736+
;; Immediately close connection so that we get a
1737+
;; non-timeout proxy connection error on the client-side.
1738+
(s/close! stream))
1739+
{:port 0
1740+
:shutdown-timeout 0})]
1741+
(binding [*connection-options* {:proxy-options {:host "localhost"
1742+
:port (netty/port proxy-server)}}]
1743+
(with-server proxy-server
1744+
(try
1745+
@(d/timeout! (http-get "/string") 1000)
1746+
(is (= true false) "Should have thrown an exception")
1747+
(catch Exception e
1748+
(is (instance? ProxyConnectException e)))))))))
1749+
17131750
(deftest ^:leak test-leak-in-raw-stream-handler
17141751
;; NOTE: Expecting 2 leaks because `with-raw-handler` will run its body for both http1 and
17151752
;; http2. It would be nicer to put this assertion into the body but the http1 server seems to

0 commit comments

Comments
 (0)