Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upMiddleware fails to propagate own exceptions in async mode #403
Comments
|
Raising an exception is fine, so long as it is caught within the calling thread. Consider: (defn handler [request respond raise]
(future
(try (respond (make-response request))
(catch Exception ex (raise ex)))))If we apply middleware to this, any exception thrown within the future will be converted to a raise. It's generally simpler to wrap the new thread than every piece of middleware. It may be an idea to automate this pattern. Either: (defn respond-with-raise [respond raise response]
(try (respond response) (catch Exception ex (raise ex))))Or: (defmacro with-raise [raise & body]
`(let [raise# ~raise] (try ~@body (catch Exception ex# (raise# ex#)))))Now, I understand that this is one more thing that the end developer needs to consider when working directly with Ring, but I don't see a good alternative. In Java, if you open up a new thread, either explicitly or implicitly, you have to have some way of handling exceptions in that thread. Even if middleware automatically handled this case, the end developer would still need to handle it for their code. What alternative design for asynchronous handlers did you have in mind? |
|
Experience developing await-cps suggests it's not that simple. The code that executes as a part of (defn handler [request respond raise]
(future
(try (respond {:status 200})
(catch Exception ex (raise ex)))))
(defn wrap-xxx [handler]
(fn [request respond raise]
; first middleware chooses to raise on respond
(handler request (fn [_] (raise (ex-info "xxx" {}))) raise)))
(defn wrap-yyy [handler]
(fn [request respond raise]
; second middleware's raise handler fails
(handler request respond (fn [ex] (println "yyy") (throw (ex-info "oops" {}))))))
(-> handler
wrap-xxx
wrap-yyy
(apply {:request-method :get} println println nil))
=> #object[...]
yyy
yyyWhile it is possible to redefine the 3-arity contract so that handlers are allowed to throw, outside responding and raising, it only solves the problem on the way down to the handler. Middleware still needs to handle the exceptions in callbacks. Also, note that this would too be a breaking change. I don't think the problem is related to Java threads. The issue is not that the unhandled exceptions silently get swallowed (this is annoying but there are workarounds), but that they don't correctly propagate via
Let me answer that in a separate comment. |
If the error handler isn't robust, yes, but robustness is a criteria of a good error handler. An error handler should handle any reasonable exceptions that arise from its operation. I also don't anticipate middleware to need to override
There's no contract that says that 3-arity handlers cannot throw exceptions; it's just not a good idea to do so if you switch threads because those exceptions won't be caught. |
|
Your reply sounds a little bit dismissive. I'm under the impression that, going with continuations, Ring is trying to avoid choosing an abstraction for asynchronous processing, but I believe that, unintentionally, without understanding the implications, it created one. With the pattern of embedding 1- and 3-arity handlers in a single function it is implied that the two have equivalent semantics. In synchronous Ring exceptions propagate by virtue of the call stack. The callback model of 3-arity subverts that, replacing the natural call stack with continuations, where the call stack is effectively inverted. It's a reasonable expectation that the semantics of error propagation of a synchronous handler translate reliably to the asynchronous counterpart. If Ring is not precise about the way to achieve it, correct software is impossible.
While I agree that that error handler shouldn't ever throw I think this is not about robustness but correctness. The duplicated error handler execution is just one example. I don't believe it's possible to define a continuation based contract that's consistent with semantics of a regular call stack, where continuations are allowed to throw. If such consistency is not a goal it really ought to be documented in the big print as it has limitations not only for those that choose to invoke callbacks manually but equally for those that use adapters to abstractions that do pursue that consistency.
As far as 'reasonable' use-cases go even rendering of an error page sometimes blows up. But again, I don't believe this is about reasonable use-cases but the general case. Error handlers do work of arbitrary complexity. Middleware is not just for libraries - end users abstract certain parts of their software with it too. Allowing one faulty middleware to have a knock-on effect on the correctness of the whole stack is the opposite of resilience. To be clear, in the model with catch-all per thread that you proposed, respond handlers that throw are prone to problems just as much. Even though I'm happy to provide examples, bear in mind that a contract that is hard to reason about is a minefield of similar problems. (defn wrap-finally [handler]
(fn [request respond raise]
(handler request
(fn [response] (println "finally") (respond response))
(fn [ex] (println "finally") (raise ex)))))
(defn wrap-xxx [handler]
(fn [request respond raise]
(handler request (fn [_] (throw (ex-info "xxx" {}))) raise)))
(-> handler ; the same as before
wrap-finally
wrap-xxx
(apply {} println println nil))
=> #error{}
finally
finally
As stated at the top of this issue, I inferred that through the behaviour of jetty adapter. Perhaps it's a bug and throwing is permitted. Either way, Ring needs to be clear which is the case as both carry extra responsibilities - catch-all in all middleware or catch-all in every new thread. I used the word 'redefine' as a decision to go with either could break existing clients. |
This perhaps deserves a separate issue but I'm going to try to be brief. Other platforms foster correctness using a construct of a potentially failed value wrapper, that forces the use of monad-like 'bind' operation to unwrap the value, and re-wrap any new failures that this might generate. A handler would either return that wrapper, or throw trying. Note that the latter bears all the composability guarantees thanks to 'bind'. For Clojure, available options I can think of are manifold Deferred, promesa IPromise and Java 8 CompletableFuture. core.async is not such option as it lacks error semantics leading to similar problems as the one being raised here. A third party dependency in the contract is undesirable making manifold and promesa a hard sell. I think that leaves the platform built-in CompletableFuture. Java 8 has been a requirement for long enough that I think it is a safe choice in that regard. The API of CompletableFuture or the interface of CompletionStage is admittedly unnecessarily convoluted. There are popular Clojure libraries that wrap it, like promesa. Even manifold works with CFs, although it requires manual conversion back. There are projects aiming to implement async/await on top of CF. hato is a Java 11 http client that speaks CF natively. A separate problem is the distinction between synchronous and asynchronous middleware. 3-arity solves the problem neatly but with CF that's not an option. This is pure speculation but I'm thinking allowing the handler to return a CF or regular response would be an option if Ring shipped with a (defn response-bind [cf-or-map f] ; f, just like a handler, returns cf-or-map
(if (instance? CompletionStage cf-or-map)
(.thenCompose ^CompletionStage cf-or-map
(reify Function
(apply [_ response]
(let [cf-or-map' (f response)]
(if (instance? CompletionStage r')
cf-or-map'
(CompletableFuture/completedFuture cf-or-map'))))))
(f cf-or-map)))
(defn wrap-xxx [handler]
(fn [request] (-> request xxx-request handler (response-bind xxx-response))))Disclaimer: not tested. Catching of errors not covered, although they propagate correctly as it stands. |
That wasn't the intent, but you do need to make a very good case to change existing behaviour, and I need to play the part of the defense and question everything.
I don't believe that's implied. When I see callbacks, I don't assume that they will act the exact way as a synchronous function would, particularly when comparing error handers to try-catch blocks.
Examples are necessary to ground any discussion. If you say something is hard to reason about, that's just opinion unless you back it up with use-cases that demonstrate unintuitive or complex reasoning. Regarding your That said, is this a use-case that will be encountered regularly? Again, the only realistic use-case I can think of for overriding
The Jetty adapter displays a 500 error response in either case, whether through |
My initial impression is that CF does not look particularly straightforward, but I'll reserve judgement until I see a more complete implementation of what would be needed for a CF-based handler/middleware architecture. |
My bad. I was
Yes, Here's another example, showing that (defn wrap-catch [handler]
(fn [q r e] (handler q r (fn [ex] (r {:status 500})))))
(defn wrap-println [handler]
(fn [q r e] (handler q (fn [response] (println "here") (r response)) e)))
(defn wrap-fail [handler]
(fn [q r e] (handler q (fn [response] (throw (ex-info "fail" {}))) e)))
(-> handler ; as before
wrap-catch
wrap-println
wrap-fail
(apply {} println println nil))
=> #object[...]
here
here |
Conceptually CF is equivalent to JS Promise that a lot of developers are familiar with. The API is incredibly bloated because Java's proliferation of functional interfaces, but I believe everything else can be expressed by a combination of
In terms of |
To clarify my position, I don't believe that asynchronous handlers need to act identically to synchronous ones in all circumstances. If there are conditions where the behaviour diverges, that's not an automatic failure, especially if those conditions are unlikely to be experienced in normal usage. That's not to say that a waterproof abstraction is something we don't want to aim for, but that needs to be weighed with other concerns. A leaky but simple abstraction may be preferred over a watertight but complex one.
So long as it's clear that any PR is going to take some time to be merged, if indeed it is merged at all. However, at least initially I'd be most interested in examples of usage. How does it compare with the current system for ease of use? Also be aware that Ring needs to continue supporting the existing behaviour, so any alternative asynchronous system needs to be in addition to the existing 3-arity behaviour, even if the latter is ultimately deprecated. This may have implications for performance and complexity. Ultimately we need to demonstrate that the existing system has enough problems in real world usage to justify any additional system. This is possible, but it's not going to be an easy sell. |
The way I understand it, leakiness is what makes abstractions complex. When you're forced to consider what other components might do, the composability suffers. Simplicity is the lightweight state of confidence in non-interaction.
Makes perfect sense. I understand the proposal around Back to 3-arity, would you agree that the |
In this case, the leakiness stems from the expectation that synchronous middleware will act the same as asynchronous middleware, but as you point out, there are edge cases where this is not true. However, synchronous middleware doesn't interact with asynchronous middleware. There's no additional complexity by supporting both types, as there's no interaction between calls. If Ring only supported the 3-airty form, there'd be no leakiness, because there'd be no expectation that the callbacks provided would act the same as a try-catch block.
No, in fact quite the opposite. Why would you put an error reporting handler at the bottom of your stack? It should be the outermost middleware. In general I can think of two scenarios you'd want to override Also, do problems only occur when there are additional side-effects in |
|
You seem to be defending the pattern with handlers catching and raising callback exceptions pretty strongly. Does that mean you consider it to always have been a part of the contract, and not a new responsibility being pushed on the handler? Or is it that you deem that responsibility to be so much easier to satisfy than the expectation for middleware callbacks not to throw, that you're open to accept the edge cases?
The difference is subtle and unintuitive. Your statement would be fair if Ring was upfront about these quirks and limitations. I wonder how much aware had the Ring community, including yourself, been before this issue have been raised? How many developers would be as willing to sacrifice composability, had they been informed?
I think it's important to distinguish errors from exceptions. Unhandled exceptions indeed are errors - there's not much you can do about them but report and it makes sense to handle them at the outermost place. But then, there are all the other exceptions that developers throw, anticipate and handle as appropriate. There are few assumptions we can make as to where these originate in and what is the right way to address them. That depends on business requirements, architecture and developer's imagination, limited by any contracts. Now, middleware is a pattern for extracting a piece of logic from a handler for reuse or code structuring, an abstraction. Developers use that pattern to implement all sorts of behaviour. Sometimes that behaviour involves the handling of exceptions. For instance, buddy.auth uses exceptions to communicate declined authorization. In the current form it only handles the ones raised in the same thread. It's not hard to imagine the intent to handle the ones raised asynchronously however, either for completeness of the The point is, I don't think it's possible or useful to guess what users do with middleware. The ones you see published in libraries are just the tip of the iceberg that happen to be reusable. Unless the contract states clearly that middleware is not to turn a Certainly, 1-arity Ring does not ask what developers do with it. That's what frameworks do. I, for one, was attracted to Ring precisely for this reason. |
There's no other way it could work. If you have code that may execute outside the calling thread and is intended to return a response, you have to catch any unhandled exceptions regardless. If you don't, exceptions thrown by that code will cause the handler to time out: (defn handler [request respond raise]
(future (/ 1 0) (respond {:status 200}))
I'm not sure I understand. A callback obviously won't work the same way as throwing an exception. I don't believe that anyone who seriously looked over the original proposal was under any illusions that there wouldn't be differences.
Let's delve a little more into that. When is it appropriate to catch an exception? The buddy.auth middleware uses exception for control flow, but I'm cautious about encouraging this, as it's generally considered to be an anti-pattern. Outside of the internals of complex macros like core.match, I haven't seen this pattern commonly used in Clojure, and I'm inclined to go with the common wisdom in this case. Another possible scenario is some form of fault tolerance middleware. We could imagine some middleware that watches for specific exceptions and retries the handler on error. For example, with again: (defn wrap-again [handler options]
(fn [request]
(again/with-retries options
(handler request))))Is this a feasible pattern? I haven't seen this in the wild, and it doesn't seem a particularly good idea to retry the entire handler, rather than a narrower portion. A third possibility is cleaning up resources on error. If there's some middleware that opens a connection or some other resource, and needs to clean up that resource after completion: (defn wrap-resource [handler options]
(fn [request]
(let [resource (open-resource options)]
(try (handler request)
(finally (close-resource resource))))))This seems the most plausible, though I admit I haven't actually seen this pattern in practice. Database connections are generally pooled, or else opened and closed within the handler. Can you think of any other scenarios? I'd like to get an idea of realistic use-cases where asynchronous handlers may potentially behave in an unintuitive fashion, and I believe that implies some form of overridden
If we cannot guess what users do with middleware, then we have to rely on user reports. But there have been no reports to my recollection, aside from yours, of any unintuitive behaviour relating to the 3-arity form of handlers. And there have been no reports of problems that have occurred in real-world production systems. Now it's possible that few people are using Ring's asynchronous handlers compared to other solutions, like Pedestal's interceptors, or manifold in Aleph. I have no way of knowing to what extent a particular feature of Ring is used. I would expect that after three years from release, if there were significant problems in practice that I'd get at least one issue or email, but that expectation may be based upon faulty assumptions. However, we cannot replace an existing system without some evidence that there problems in real-world use-cases. If there are no user reports, then due dilligence would suggest that we at least try to come up with plausible scenarios where there could be a problem. |
The question is specifically about exceptions bubbling from the callbacks. While the responsibility to I was a bit confused when I first realised the implications of a throwing callback, but couldn't find any write-ups on it, so I went on to assume they're not supposed to do that. I guess your replies suggest I was wrong. But still I wonder, how many developers might have assumed the same thing? How many developers didn't give it enough thought and failed to wrap I wasn't able to find many async Ring examples online, but here is one from a popular library where callback exceptions are ignored. Incidentally, your recent response ignores that subject too, although I understand it's just a snippet on reddit. Any chance you can point me to a code base/example that gets it right? I couldn't find anything else.
So is callback hell, yet here we are. Exceptions are a language construct. Given you support them kind of, it would be nice to support them consistently, or loudly deny it. No one considers 1-arity Ring to be encouraging this pattern either, but the fact it works is a reasonable expectation. As this article illustrates, exceptions are great if they're used for exceptional cases.
Pooled connections need a reliable release all the same.
(defn wrap-unavailable [handler service-name]
(fn [q r _]
(handler q r
(fn [ex] (r {:status 500 :body (str service-name " is unavailable")})))))
(defn proxy-handler [match proxy-to]
(fn [{:keys [uri]} r e]
(if (clojure.string/starts-with? uri match)
(clj-http.client/get (str proxy-to uri)
(fn [response] (try (r response) (catch Throwable t (e t))))
e)
(r nil))))
(defn app-handler
(compojure.core/routes
(wrap-unavailable (proxy-handler "route-a/" "http://service.a/") "Service A")
(wrap-unavailable (proxy-handler "route-b/" "http://service.b/") "Service B"))) Again, this example illustrates that developers may want to turn a
Judging by the volume of online articles, the asynchronous variant is not too popular. Together with the fact that the standard middleware doesn't throw unless you fiddle with it, this doesn't surprise me. I would argue this doesn't make it any less of an issue, even if of a lower severity. Not unlikely the stability of some of those systems relies on
Excuse me if I'm taking my time separating the CF proposal. As far as the scope of this report goes, the system doesn't need replacing, just patching. If middleware-injected logic handled its exceptions the problem would be alleviated. The hard part is making developers aware of this requirement, having them adjust existing libraries and not failing at it. The CF proposal is partly inspired by the inability to enforce this requirement (if it's a requirement at all) |
Ah, by so "callback exceptions" you meant specificially the I can appreciate the reasoning behind that. In a synchronous model the But should this pattern be mandated? And what happens if only
You'll need to justify callbacks being generally considered an anti-pattern.
Yes, but the acquire and release for pooled connections is typically brief; if you're using a pool, you tend to release the moment you're done with the connection. I wouldn't expect to see middleware releasing a pooled resource.
Can you give an example? I know that clj-http throws exceptions by default on error responses, but that's not control flow unless those exceptions are expected.
Breaking out exception handling of the proxy handler into its own middleware seems an odd design choice, but it's possibly the closest we've come yet to a realistic use-case.
One possible course of action could be to add functions to make it easier to wrap exceptions in callbacks. I mentioned this earlier, but something applied specifically to middleware: (defn catch-raise [raise callback]
#(try (callback %) (catch Throwable t (raise t)))))
(defn wrap-example [handler]
(fn [request respond raise]
(handler request (catch-raise raise (fn [resp] (assoc resp ::foo 1))) raise))) |
|
It might be a good time for me now to take my "devil's advocate" hat off, as I think we've covered the main arguments of this discussion, and we're sinking into details that I don't think will affect the outcome. I'd like to summarize both the problem and my current position. The problem occurs under the following circumstances:
This can result in an inner This seems a fairly rare problem in practice, and my current feeling is that this does not require replacing the existing asynchronous handler design in its entirity (e.g. with CF or similar). However, I do think it justifies some helper functions, and for a stricter specification, as you convincingly argued for. So I'd like to propose the following:
The (defn raising [raise f]
(fn [& args] (try (apply f args) (catch Throwable t (raise t)))))For example, middleware that changes the (defn wrap-content-type [handler options]
(fn [request respond raise]
(handler request
(async/raising raise
(fn [response]
(respond (content-type-response response request options)))
raise)Do you see anything I've missed? |
The symptoms observed depend on the error-handling strategy. So far we've discussed a handler that catches both, own exceptions and those that bubble from (defn simple-handler [q respond raise]
; Don't bother handling errors if you can prove your code doesn't throw.
(future (respond i-never-throw)))
(defn precise-handler [q respond raise]
; Make sure to handle any failures on the handler side,
; then invoke callback with no safety nets.
; Roughly equivalent to what compojure.api does with its Sendables
(future (if-let [result (try (risky-business) (catch Throwable t (raise t) nil))]
(respond result)))
(defn catch-all-handler [q respond raise]
; Catch all unhandled exceptions and raise, as discussed so far
(future
(try (respond (risky-business))
(catch Throwable t (raise t)))))
(defn handling-handler [q respond raise]
; Actually handle an exception
(future
(try (respond (risky-business))
(catch ExceptionInfo e (respond (error-response e))))))In principle, any code path that might invoke callbacks multiple times or fail to invoke any, is going to exhibit faulty behaviour in some way. Allowing
In my opinion, the problem is rare in practice because middleware rarely throws. Most async handlers I've seen would be affected in some way if it did. I would argue Ring ought to either mandate callbacks non-throwing to make reasoning about handler code paths easy, or instruct users precisely how to handle errors/exceptions right. The former appeals to me much more and, the way I see it, is just formalising what's already a common practice. Your proposal looks good. It's exactly what I was hoping to achieve. Thanks!
The remaining shortcoming is the inability to enforce the rules imposed. It's arguable if it's worth introducing an alternative contract but I'm willing to take my shot in a separate GH issue. |

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

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:
If
xxx-requestorxxx-responsewas 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 beingraise-d.Example:
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
encoderis 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):
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
-requestand-responsefunction and maybe some exception processor, similar to the interceptor pattern. I'm happy to propose an implementation if you're interested.