weavejester / compojure

A concise routing library for Ring/Clojure
Eclipse Public License 1.0
4.08k stars 259 forks source link

Using 100s of routes with ring async handlers causes StackOverflowError #187

Open vincentjames501 opened 5 years ago

vincentjames501 commented 5 years ago

We have a large ring/compojure application with lots of api routes. We are trying to use ring's async handlers but we eventually run out of stack if a matching path is not found in time.

(let [handler (apply compojure/routes
                    (repeat 3000 (compojure/GET "/foo" [] {:status 200 :body "bar"})))]
  (handler {:uri "/foo2" :request-method :get}
           println 
           println))
=> java.lang.StackOverflowError: 

This is fine because it matches a route soon enough

(let [handler (apply compojure/routes
                    (repeat 3000 (compojure/GET "/foo" [] {:status 200 :body "bar"})))]
  (handler {:uri "/foo" :request-method :get}
           println 
           println))
=> {:status 200, :headers {}, :body bar}
=> nil

The issue is that it will create a new entry in the stack for each handler that doesn't match:

(defn routes
  "Create a Ring handler by combining several handlers into one."
  [& handlers]
  (fn
    ([request]
     (apply routing request handlers))
    ([request respond raise]
     (letfn [(f [handlers]
               (if (seq handlers)
                 (let [handler  (first handlers)
                       respond' #(if % (respond %) (f (rest handlers)))]
                   (handler request respond' raise))
                 (respond nil)))]
       (f handlers)))))

Even if you break it into chunks like so, you still get the same error.

(let [handler (apply compojure/routes
                     (map (fn [_] (apply compojure/routes (repeat 30 (compojure/GET "/foo" [] {:status 200 :body "bar"}))))
                          (range 100)))]
  (handler {:uri "/bar" :request-method :get}
           println 
           println))
=> java.lang.StackOverflowError: 

This can be rewritten using something like a go loop, but it's possible code is blocking and may unknowingly block peoples core async threads.

Any suggestions?

vincentjames501 commented 5 years ago

An idea (though not bullet proof), we could make wrap-route-matches return something synchronously like ::compojure/no-match and tweak routes like so?

(defn routes
  "Create a Ring handler by combining several handlers into one."
  [& handlers]
  (fn
    ([request]
     (apply compojure/routing request handlers))
    ([request respond raise]
     (letfn [(f [handlers]
               (loop [[handler & remaining-handlers] handlers]
                 (if handler
                   (let [respond' #(if % (respond %) (f remaining-handlers))]
                     (when (= (handler request respond' raise) ::compojure/no-route-found)
                       (recur remaining-handlers)))
                   (respond nil))))]
       (f handlers)))))

It can technically still run into the same issues if lots of routes match though the chances are slim.

weavejester commented 5 years ago

That's an interesting problem. As a temporary workaround, I'd suggest increasing the stack size for the running JVM, since you're not dealing with an unbounded stack.

Solving the problem in a more general way will require a bit of thought.