Add support for concurrent request limits and queue size

[rutchkiwi]

Fixes #39
Works fine as is, but it seems odd that this kinda but not really duplicates the :worker-threads option. I need to think though, or discuss, how that should work. I feel like there might be an obvious answer.
Also cleaned some minor things, I can split that out into a separate commit, or skip it depending on your thoughts.

[yogthos]

Not sure there's a better way to do it. Worker threads are a subtly different way to tune concurrent throughput based on worker availability.

[rutchkiwi]

is there any case when you would want more worker threads than concurrent requests, or vice versa? Im thinking to drop the concurrent requests parameter, and set the max concurrent request to worker-threads? (only if queue size set, otherwise no changes to how it is.

[yogthos]

I think they're just different ways to do limiting. With max requests you control the queue of requests before you start dropping them, so even if you have a single worker you can limit how many requests you allow to build up. Meanwhile, the worker threads control how many requests you're processing at a time.

[rutchkiwi]

Hmm. As an example:
If you have say 10 worker-threads, 15 max-concurrent-requests, and then request-queue-size 3.
wouldn't you in effect just have a queue of (15-10)+3, so 8?
15 requests would go through the RequestLimitingHandler, but since you only have 10 worker threads, the remaining 5 requests would sit in some internal queue. And 3 extra requests hang out in the queue outside of RequestLimitingHandler?
Considering that, I think the correct thing would be to have :max-concurrent-requests default to :worker-threads. But it should be allowed to be specified separately for maximum control - the only case I can think of when this would be relevant is for :async? handlers. (I'll mention that in the doc).
Does that seem correct? If so I'll start working on it.

[yogthos]

I don't think you'd want to use all of these settings together though. In practice, you'd probably pick one of them based on what you're aiming to limit. So, I think it is about maximizing control for the user where they can choose what knobs they want to tune. I wouldn't conflate different settings implicitly though. I think if the user wants to set max-concurrent-requests they should do it explicitly in the config. Having that being set implicitly by worker-threads creates internal coupling that's opaque. In my opinion, the PR is good as is, I'd just document the new flag that's available.

[rutchkiwi]

I agree that you want to be able to tune each knob independently. The issue I'm concerned about is exactly that - someone (like me a month ago) wanting to limit the queue, and only setting the :request-queue-size.
In the current implementation, only setting :request-queue-size, without setting :max-concurrent-requests at the same time, has no effect on the queue size, which will be unbounded anyway.
So that internal opaque coupling is already there, and comes from the current implementation.
This could of course be documented, but it feels like quite a footgun that is likely to fire just when you don't want it (overloaded server).

Doing some (AI) research, I believe the only way to limit the queue size directly would be to make a completely custom handler. Which seems hard and potentially error prone..

So anyway, as long as we're on the same page about that nastiness with :request-queue-size not working without :max-concurrent-requests I don't mind doing it either way:

• documenting this gotcha,
• and/or printing a warning when :request-queue-size only is set, or
• having :max-concurrent-requests default to :worker-threads size when only :request-queue-size is set.

Please let me know your thoughts

[yogthos]

I'm thinking let's document the gotcha, and another option could be to add a new flag that does explicitly set both. We could mention in the docs that that's the sensible default, something like :max-threads-and-concurrent-requests.

[rutchkiwi]

I like that, maybe just :concurrent-request that sets both (And skip the option to only set only RequestLimitingHandler)?
I'm also thinking to refuse to start the server if :queue-size is set but RequestLimitingHandler.concurrent-requests doesn't have a value to be set to?

[yogthos]

Sure let's do that, seems reasonable. And if somebody has a use case where they need more freedom can always add more options later.

[rutchkiwi]

Ok, I've updated the PR now.
I also found that the readme was getting a bit out of hand will all the options, so I structured that up a bit. I know it's not really related to the purpose of the PR - I can remove it from PR if you want, or remove it when merging if we're there.

[yogthos]

The readme update looks good thanks, and just pushed out 1.4.4 with the new features. 🎉

[rutchkiwi]

Thanks!!!
This was my last blocker for migration my main website (https://www.medino.com/) over to undertow from http-kit.
I'll test it out and let you know if I find any issues. :)

[yogthos]

Awesome, definitely keep me posted on how things go. Always great to see the libraries actually get used for real world stuff like that. :)