Skip to content

Middleware fails to propagate own exceptions in async mode #403

@mszajna

Description

@mszajna

The contract of Ring async handlers is to eventually either call respond or raise. The Concepts page doesn't go into that much detail but I infer there is no semantics attached to a handler throwing an exception given jetty adapter ignores them.

ring.middleware.* follows the pattern of separating request and response processing of:

(defn wrap-xxx [handler options]
 ([request respond raise]
  (handler (xxx-request request options)
           (fn [response] (respond (xxx-response response options))) raise)))

If xxx-request or xxx-response was to throw, because of a bug, an unprocessable request/response value, as a result of faulty options or executing code supplied in the options, the exception bubbles instead of being raise-d.

Example:

((ring.middleware.cookies/wrap-cookies
  (fn [_ respond  _] (respond {:cookies {:a 1}}))
  {:encoder (fn [_] (throw (ex-info "fail" {})))})
 {} println println)
=> Execution error (ExceptionInfo) fail
; expected
=> nil
Execution error (ExceptionInfo) fail

All middleware following this pattern that cannot guarantee not to throw is affected. Frankly, rarely any useful code can make such guarantees. It would be easy to dismiss the problem on the grounds of breaking the contract, saying encoder is not allowed to throw for example. In practice, real world code throws in unexpected ways - what suffers here is resilience.

The compromised pattern seems to have been picked up by third party libraries.

It's actually rather tricky to comply with the contract. It requires careful scoping of try-catch blocks (made less cumbersome with try-let):

(defn wrap-xxx [handler options]
 ([request respond raise]
  (let [[request' ex] (try [(xxx-request request options)]
                        (catch Throwable t [nil t]))]
    (if ex
      (raise ex)
      (handler request'
               (fn [response]
                 (let [[response' ex] (try [(xxx-response response options)]
                                        (catch Throwable t [nil t]))]
                   (if ex
                     (raise ex)
                     (respond response'))))
               raise)))))

Apart from this being a genuine bug, I wanted to raise the issue as a demonstration of how hard it is to comply with the 3-arity contract even in the simple case, and what's even worse, decide if any given implementation adheres to it or not. The issue makes question whether the contract's complexity had been fully considered, and the burden of verifying compliance acknowledged before adoption. If the assumption was that users won't interact with callbacks directly, the ecosystem of faulty libraries begs to differ.

With Ring 2.0 updating the contract anyway, you are in position to reconsider this decision. I understand it's a very difficult decision to make, with the Clojure community's continued failure to arrive at a single, widely adopted standard. Still, I believe avoiding commitment to any particular solution proved to introduce more problems than it mitigates.

Lastly, for Ring 1.x middleware authors, and perhaps for the benefit of this library, you could introduce a middleware factory taking a synchronous -request and -response function and maybe some exception processor, similar to the interceptor pattern. I'm happy to propose an implementation if you're interested.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions