Missing API to increase QueuedThreadPool maxThreads by leased threads + QoSHandler bug of exceeding maxRequestCount by 1

[scscgit]

Jetty version(s)
12.0.15

Java version/vendor
openjdk-23.0.1

OS type/version
Windows 10

Description
When configuring a QueuedThreadPool for the Server, we may have one practical dilemma:

If we limit the server by a number of requests, i.e. using new QoSHandler().setMaxRequestCount(1), we also need to decide what number of threads to allow. If our handler is single-threaded, our educated guess is to reserve one thread per request.

However, Jetty Server also runs internal threads in this pool. The implementation in org.eclipse.jetty.util.thread.ThreadPoolBudget#check makes sure there is enough space for all these so-called "leased" threads. My concerns are:

1. There is no API (or a constant) to retrieve the number of (expected) leased threads, so we can use it to add to the QueuedThreadPool's maxThreads value. Alternatively, there is no way to specify how many "handler threads" there should be, so that the leased threads would be created automatically.
2. The Thread Pool documentation (mentioned in QueuedThreadPool throws error on quick successive execute calls if initialized with BlockingArrayQueue or ArrayBlockingQueue #13004) claims that "the server implementation may produce a number of tasks that must be run by the thread pool". It's not explicitly explained when and how many threads may need to run at any time. If we specify our maxThreads as leased threads + handler threads, the maximum number of available handler threads may decrease, even below zero.

In practice, we'll usually set the maxThreads to a value like handler threads + 8, but this is not a portable solution, and is possibly wasteful. If Jetty implementation changes the number of leased threads in a future version, hard-coded constants may cause our application to misbehave.

Additionally, there is one specific bug of QoSHandler, which can be reproduced if the maxThreads is set to exactly leased threads + 1 (adding one more thread fixes the problem).

The steps are to set maxThreads to 4 (assuming there are 3 leased threads), and configure:

• qosHandler.setMaxRequestCount(1);
• qosHandler.setMaxSuspendedRequestCount(0);

This should mean there is exactly one thread available to process exactly one request, with no queue for pending requests.

Expected output should be 1 OK response 200 and an instant rejection of 2 other requests with an error response 503.

Actual result: 2 responses 200 and 1 response 503.

Started
Started Handle
Thread 1 ended: 503
Finished Handle
Started Handle
Thread 2 ended: 200
Finished Handle
Thread 3 ended: 200

And actually, it's not even necessarily exactly 1 success result. With 20 threads, I reproduced 2 errors and 18 successes.

Plus I'd like to confirm one more thing: if I replace new Handler.Abstract() by new Handler.Abstract.NonBlocking() in the same code, the maxRequestCount of QoSHandler is completely ignored, as if maxSuspendedRequestCount were infinite. Hopefully that's by design.

Thanks.

How to reproduce?

import org.eclipse.jetty.server.*;
import org.eclipse.jetty.server.handler.QoSHandler;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.thread.QueuedThreadPool;

import java.io.IOException;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;

    public static void main(String[] args) {
        final int leasedThreads = 3;
        QueuedThreadPool queuedThreadPool = new QueuedThreadPool(leasedThreads + 1, 1, 2000);
        queuedThreadPool.setDaemon(true);
        Server server = new Server(queuedThreadPool);
        ServerConnector connector = new ServerConnector(server);
        connector.setHost("localhost");
        connector.setPort(8081);
        server.setConnectors(new Connector[]{connector});
        QoSHandler qosHandler = new QoSHandler();
        qosHandler.setMaxRequestCount(1);
        qosHandler.setMaxSuspendedRequestCount(0);
        qosHandler.setHandler(new Handler.Abstract(){
            @Override
            public boolean handle(Request request, Response response, Callback callback) throws Exception {
                try {
                    System.out.println("Started Handle");
                    Thread.sleep(1000);
                    System.out.println("Finished Handle");
                    callback.succeeded();
                    return true;
                } catch (InterruptedException e) {
                    throw new RuntimeException(e);
                }
            }
        });
        server.setHandler(qosHandler);
        try {
            server.start();
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
        System.out.println("Started");
        for (int i = 0; i < 3; i++) {
            final int index = i + 1;
            try {
                Thread.sleep(100);
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
            new Thread(() -> {
                try {
                    HttpResponse<String> response = HttpClient.newHttpClient().send(
                            HttpRequest.newBuilder()
                                    .uri(URI.create("http://localhost:8081"))
                                    .GET()
                                    .build(),
                            HttpResponse.BodyHandlers.ofString()
                    );
                    System.out.println("Thread " + index + " ended: " + response.statusCode());
                } catch (IOException | InterruptedException e) {
                    System.out.println(e.getMessage());
                    throw new RuntimeException(e);
                }
            }).start();
        }
    }

[sbordet]

Why are you trying to limit the number of threads in this way?

It would be much better if you let Jetty handle the threads, and you just configure a reasonable maxThreads that is not based on minimal assumptions.

In practice, we'll usually set the maxThreads to a value like handler threads + 8, but this is not a portable solution, and is possibly wasteful.

Why +8? Why is it wasteful?
Jetty won't create threads that are not needed.

There are 3 Jetty components that lease threads from the ThreadPool: the selectors, the acceptors, and the reserved threads.
You can configure explicitly these 3 to know exactly the number of leased threads.
There is no API because there is no way for Jetty to know beforehand how many connectors you configured, and for each connector how many selectors and acceptors.
But your application knows, so it can do the math.
We will document this a bit better via #4493.

Plus I'd like to confirm one more thing: if I replace new Handler.Abstract() by new Handler.Abstract.NonBlocking() in the same code, the maxRequestCount of QoSHandler is completely ignored, as if maxSuspendedRequestCount were infinite. Hopefully that's by design.

I'm not sure what you mean exactly above, but QoSHandler.maxRequestCount is not ignored.
Likely you are using 1 selector only, so with Handler.Abstract.NonBlocking requests are serialized one after the other, as expected.
Is that the behavior you are seeing?

As for the QoSHandler bug, will investigate.

[scscgit]

@sbordet Thanks, I understand we may typically allow more maxThreads than exactly just leased + requestCount, but "more" is still ambiguous, like, should the upper boundary be 2-times the request count? Or 10-times? Or infinite? We run many servers in the same application, and we need to establish some boundary for many of them. Assuming our handlers are single-threaded, it doesn't make much sense to allow infinite thread consumption. And in some servers, there may be a use-case where the absolute minimum of resources have to be reserved (so I illustrated an example with exactly 1 handler thread).

When adding "some buffer", +8 is a safer choice than +3 -> some constant just has to be added, given a usage of configurable parameters (that we don't know upfront) in the equation. Having an API available would ensure we avoid startup errors or logical mistakes (like allowing less threads than requests).

• Actually, during the startup, my sample code enforces that maxThreads needs to be 4 or more, but with maxThreads value 300, queuedThreadPool.getMaxLeasedThreads() returns 23 (queuedThreadPool.getLeasedThreads() returns 7). So adding a constant is not really okay anyway.
  • This number is typically constant after this thread pool is {@link #start() started}. -> only typically? :)
• I understand some components with leased threads may be added after construction, but there is also the common case where we don't add any custom components, in which case a sample code to extract the number from an instance would be useful.
• Personally, I'd suggest providing a code we can call after the construction but before starting the server, which will increment the maxThreads by the leased threads count.
• Still, except these 3 components, we need to know if Jetty runs any other code with the same semantics like leased threads, as that is hinted by the documentation I mentioned. That's why, as a user, I don't want to rely on having to scavenge documentation for components, acceptors, or leases, but rather I want to see the intention of "how to retrieve the number of all threads Jetty will use internally". It will be great if your opened documentation task covers this perspective (because Jetty is very poorly indexed by search engines, so we can't even find such questions).

Regarding the Handler.Abstract.NonBlocking, I only observed that it handles all requests instead of rejecting them, so the QoSHandler has absolutely no effect. There are no more selectors in the sample code. Maybe a warning could be displayed with such misconfiguration/incompatibility, but I haven't done any deeper analysis, so if that's by design and implicit, then it's okay by me.

Btw. I'm currently trying to figure out why, after 300 requests finish, within 1 minute, over 30 threads suddenly briefly appear in the thread pool. I assume those are the transient internal threads, so it would be nice to reserve enough resources for them.

[sbordet]

I have expanded the documentation in #13188.
Would be great if you can review and report if it's enough to clarify your doubts.

We run many servers in the same application, and we need to establish some boundary for many of them.

Why?
As I said, you don't need to. Jetty will only create the threads it needs.

You can configure an absolute minimum of threads, but then don't complain if things don't work 😏
So "more" is more, and depends on your case. We don't know if you have 1 concurrent request or 1 million, you have to apply some common sense based on your particular use case, which we don't know.
You have been able to establish that +8 is better than +3, so you know your system, how it behaves, and how much "more" is enough for you.

As for the API, as I said it is not available because the components are not known upfront, nor their configuration.
I have documented this in #13188, so no APIs, but it's now documented.

Actually, during the startup, my sample code enforces that maxThreads needs to be 4 or more, but with maxThreads value 300, queuedThreadPool.getMaxLeasedThreads() returns 23 (queuedThreadPool.getLeasedThreads() returns 7).

This is already documented here:
https://jetty.org/docs/jetty/12/programming-guide/arch/threads.html#thread-pool-configuration

If you want to carefully control the threads, you have to explicitly configure all components that have an impact.
Hopefully #13188 clarifies that too.

That's why, as a user, I don't want to rely on having to scavenge documentation for components, acceptors, or leases, but rather I want to see the intention of "how to retrieve the number of all threads Jetty will use internally".

I think you have the wrong perspective on this.
You should not worry in 99% of the times.
You still have not said why you need to do all this work, when doing nothing would likely achieve the exactly same.
In the remaining 1%, you are an advanced user and then yes, you need to know what you're doing and scavenge the documentation.

Regarding the Handler.Abstract.NonBlocking, I only observed that it handles all requests instead of rejecting them, so the QoSHandler has absolutely no effect.

That's not correct.
It's just that your setup will have all requests serialized.
With multiple connections, and multiple selectors, even with Handler.Abstract.NonBlocking, QoSHandler will do its job.
BTW, QoSHandler does its job also in your setup, just that you never have more than 1 request at a time.

Follow the documentation in #13188, and you will likely configure your system better, and if the new documentation is still unclear, report back (by reviewing that PR, not here -- thanks).

[scscgit]

Regarding Handler.Abstract.NonBlocking, I'm not sure what do you mean by my example not having more than 1 request at a time, this is the output I'm getting:

Started Handle
Finished Handle
Started Handle
Thread 1 ended: 200
Finished Handle
Started Handle
Thread 3 ended: 200
Finished Handle
Thread 2 ended: 200

About the "1 thread per request" scenario I'm describing in case we want to limit max threads, yeah I'd now probably vote for using a safer equation like 1.5 * requests + 3 instead of + 8 :) The needs often evolve over time, and we can't always simply agree to arbitrarily refactor a critical application with a large traffic to suddenly allow unlimited threads by all servers.

• I'll open an internal discussion and let you know if someone provides reasonable arguments for this aspect. But I'm sure some people will revisit this topic in a couple of years, so I just want to collect the details here :)

Thanks for the docs update, it's mostly relevant from the perspective of knowing which values to set to configure the leased threads, so the minimum can be set directly. However, the perspective I presented was the opposite, where a user wants to use the default lease behavior, while the max thread number is variable, and its minimum available number is to be provided.

• If we wanted to infer the selectors value, there is an unfortunate circular dependency where new ServerConnector(server, factories).getSelectorManager().getSelectorCount() requires the Server, which requires QueuedThreadPool, which requires the maxThreads value, which is internally generated via org.eclipse.jetty.io.SelectorManager#defaultSelectors, which depends on the original maxThreads. Of course, we can hard-code our custom logic, but I'm really trying to figure out a portable solution that doesn't introduce any future risks of diverging from the defaults.

• The default acceptors value is 1, so that should be easy, but we'll still diverge from the defaults if we hardcode it. In the docs, you seem to assume we usually want to intentionally decide all values by ourselves.

• The reservedThreads should be likely inferred using a public org.eclipse.jetty.util.thread.ReservedThreadExecutor#reservedThreads, which requires passing constructing one dummy QueuedThreadPool first. (And if we wanted to be technically exact, after adding all these 3 variables, we'd need to re-run the calculations with each new maxThreads value, otherwise it will differ from the Jetty's default calculated from the final sum.)

Needless to say, the primary risk is that if any of this changes during an upgrade in a couple of years, we'll have wrong values hardcoded in the project and nobody will notice it. When excluding this risk, I agree that any negative impact is at least expected to be negligible in most typical production usages (just making a few more or a few less % of threads available). Honestly, I actually hope Jetty will later implement a more intelligent/dynamic process to manage the leases by default if the current equations are just placeholders and were not yet a result of any extensive performance analysis :)

• The most common situation is that a new developer will not find a solution in the documentation, so they'll make the same mistake of adding a + 8 like I did, and then their application will be able to serve less maximum number of parallel requests than what is configured. That's just a Developer Experience problem.

Also, your documentation doesn't seem to have mentioned the internal Jetty threads that are not leased threads. These are unrelated to ThreadPoolBudget, right?

Thanks!

[sbordet]

About the output of your test, what you see in practice, both with Handler.Abstract and Handler.Abstract.NonBlocking, is correct.

The behavior is influenced by how you configured Jetty, and if you expect something different, you have the wrong expectations, or you did not understand how your configuration changes impact your test.

I have already said that trying to limit maxThreads does not seem a great idea.
You are just overcomplicating things, and you still did not say what benefit you would get.

If we wanted to infer the selectors value, there is an unfortunate circular dependency

It's not unfortunate, it's a heuristic we picked by experience.
You are free to pick your heuristic, and to make your heuristic portable.

Otherwise, start the Server, pull out the configuration after start, and call QueuedThreadPool.setMaxThreads() with the value you have calculated with your heuristics.

Needless to say, the primary risk is that if any of this changes during an upgrade in a couple of years, we'll have wrong values hardcoded in the project and nobody will notice it.

There is an easy solution: configure Jetty with your own portable heuristic, which only you can know, and you won't have any problem, nor now, nor in the future.

The most common situation is that a new developer will not find a solution in the documentation, so they'll make the same mistake of adding a + 8 like I did, and then their application will be able to serve less maximum number of parallel requests than what is configured. That's just a Developer Experience problem.

No, it's a developer changing the Jetty configuration for no good reason, without understanding the impacts, nor how Jetty works, and then complaining.

I am all for improving the documentation, but you still have not explained your use case: why you want to limit maxThreads?

[sbordet]

With your constrained QueuedThreadPool configuration, the heuristic selected 1 selector and 1 reserved thread, so there are 2 threads available for transient tasks.

The client opens 3 connections, which are accepted and registered in the only selector.
Then the client send 1 request on each connection.

The first request arrives on the server, which figures out Handler.Abstract is blocking.
Tries to check whether it can hand off selector duties to a reserved thread, but none is available (not yet created), so it queues a reserved thread and queues the request task for execution.

The task queueing causes the 3rd and 4th threads to be created; one for the reserved thread and one for the request task.

Meanwhile, the selector thread finds the 2nd request from the 2nd connection; this time a reserved thread is available, so the selector thread goes on to process the second request, and the reserved thread becomes the selector thread.

The new selector thread finds the 3rd request from the 3rd connection; tries to hand off selector duties to a reserved thread, none is available, so queues a reserved thread, and queues the request task.

Now there are 2 requests hitting the Handlers: one passes QoSHandler, but the other is rejected.

The thread that rejected one request is now free, picks up the next task, a reserved thread (that is now started and becomes idle), but the 3rd request task is still waiting in the queue.

After 1 second, the 1st request finishes, and the 3rd request can be picked up.

This explains one 503 and two 200.

@gregw @lorban thoughts on the above?
Obviously a very constrained thread pool, but the 3rd request sits in the queue even if there is a thread that could have handled it, but became an idle reserved thread.

[lorban]

@sbordet I personally believe the behavior you described is sane: Jetty needs to reserve some threads for special future requests, so those can't be used to serve new requests: they need to stay idle in case one such special request arrives.

Configuring 0 reserved thread would ease that problem, obviously at a cost in latency. But isn't this issue fundamentally about tradeoffs?

[gregw]

My 2c on this is that a server needs to be limited in some way, otherwise DOS is always a risk. Limiting by threads has always been a very poor way to do this, as while it constrains the number of stacks in memory, it does not constrain the buffers held for request data or partially written responses. In fact, lack of an available thread may cause request handling to not proceed, so buffers might well be held longer than they otherwise would be.

I would do some experiments. Put the QoSHandler in and configure to support the QoS desired. Then well test the server with unlimited threads and see what the ave/maximum number of threads are. If they are within your memory constraints, then you are done. If not, then you can look at configuring either smaller stacks for all threads, or a limit on the total number of threads. I would not set reserved threads to 0 as they are reserved to handle requests, so not allowing them to be ready for a fast hot memory handling does not increase the number of requests that can be handled.

Tl;dr; I would focus on limiting real resources like memory, rather than limiting virtual resources like threads.

[gregw]

I've created PR #13208 to address this issue (at least in part).

The substantive change is that we will no longer reserve a thread if there are jobs in the queue waiting to be executed.

I have also deprecated many (most) of the derived getters like getReady() etc. as ready doesn't mean anything - ready for a tryExecute or ready for a execute? Better to well explain the fundamental values and allows users to combine them for their specific needs.

[gregw]

@scscgit can you review the PR?