search

ring-clojure / ring

Clojure HTTP server abstraction
MIT License
3.77k stars 520 forks source link

Add more informative error when :max-queued-requests is less than :max-requests and 8 #356

Closed Akeboshiwind closed 5 years ago

Akeboshiwind commented 5 years ago

If accepted should close #354.

weavejester commented 5 years ago

Thanks for the PR. Why does :max-queued-requests need to be greater than 8? Is that hard-coded into Jetty?

Akeboshiwind commented 5 years ago

Yeah, it's the minimum value that queue-capacity can have in the threadpool buffer. See here: https://github.com/ring-clojure/ring/blob/master/ring-jetty-adapter/src/ring/adapter/jetty.clj#L105

The error could probably be worded better but I didn't want to mention queue-capacity in the error because that's an internal variable name.

weavejester commented 5 years ago

I've been looking into the reasons behind the current queuepool code, and it looks like it's designed to mirror the code in Jetty. The reason that there is an 8 minimum capacity is because that's what the QueuedThreadPool implements by default.

Now that we know this, we can see the intent is to allow a minimum default queue size even if the number of threads is very low. However, if someone is explicitly passing the :max-queued-requests option, then this default no longer applies. Therefore we should have something like:

queue-max-capacity (options :max-queued-requests Integer/MAX_VALUE)
queue-capacity     (-> min-threads (max 8) (min queue-max-capacity))

In other words:

  1. The queue capacity should be no greater than the max-queued-requests
  2. By default, the queue capacity should be the number of threads
  3. If the number of threads is below 8, then by default the queue capacity should be 8

Rule 1 takes precedence over rules 2 and 3.

So I think we can actually do away with the exceptions (sorry for telling you to write them), now that the reasons for the underlying code are better understood.

Akeboshiwind commented 5 years ago

No problem, you're correct this solution is better. Thanks for taking a more detailed look into the issue :)

I'll close this PR as presumably you will make the one line change that's needed.