Is it better to fail loudly in case of bounded queue

[honnix]

Jetty Version
12

Jetty Environment

core

Java Version
25

Question

jetty.project/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java

Lines 184 to 188 in e6dfddc

	if (queue.remainingCapacity() != Integer.MAX_VALUE)
	{
	LOG.warn("Detected thread pool queue {} bounded at {} entries, which can lead to unexpected behavior. Use an unbounded queue instead.", queue.getClass(), queue.remainingCapacity());
	_capacity = queue.remainingCapacity();
	}

here issues a warning, but with the behaviour change, maybe it is better to fail loudly so users clearly knows that they are expected to use an unbounded queue for QueuedThreadPool. I don't have any context of the change and backward compatibility need, so just asking. Thanks you.

[sbordet]

What behavior change are you referring to?

We may decide in a major version of Jetty to fail hard, but we cannot do that in non-major Jetty versions.

[honnix]

Yeah I completely agree we had wrong usage, no doubt for that. This discussion is mostly about why not making this fail loudly if it is always wrong to use bounded queue in 12. Seems like a missed opportunity, but I could totally messed up the timeline regarding this.

To be more specific, it is https://github.com/dropwizard/metrics/blob/release/4.2.x/metrics-jetty12/src/main/java/io/dropwizard/metrics/jetty12/InstrumentedQueuedThreadPool.java that we use. It's not obvious to tell from that implementation that bounded queue is wrong (it's actually sort of encouraging using a bounded one).

[sbordet]

To be more specific, it is https://github.com/dropwizard/metrics/blob/release/4.2.x/metrics-jetty12/src/main/java/io/dropwizard/metrics/jetty12/InstrumentedQueuedThreadPool.java that we use. It's not obvious to tell from that implementation that bounded queue is wrong (it's actually sort of encouraging using a bounded one).

For the record, I don't see how that class is encouraging a bounded queue.
It is just a tiny wrapper around Jetty's QueuedThreadPool, and it does not default the queue to a specific configuration.
If this is done externally in DW, I don't know, but that class in particular seem ok to me.

This discussion is mostly about why not making this fail loudly if it is always wrong to use bounded queue in 12.

It is wrong in any Jetty version.
We just took the chance to warn about it, but as I said we cannot fail until the next major release.

[honnix]

For the record, I don't see how that class is encouraging a bounded queue.

It's this part https://github.com/dropwizard/metrics/blob/5ace721a655fd27b271cc8bcbad6565263b21b17/metrics-jetty12/src/main/java/io/dropwizard/metrics/jetty12/InstrumentedQueuedThreadPool.java#L135-L137.

Anyway, just to confirm, the rejection issue using a bounded queue already exists in Jetty 11 and maybe earlier versions? We upgraded from 9 to 12 and started to see the issue until we fixed using an unbounded queue, which means 9 was not too bad some how, again I understand it is a wrong usage regardless.

I'm asking these questions because we have a bunch of other services to upgrade, so we want to get a better picture what we need to fix and what our expectation should be after the upgrade.

Thank you for answering all my questions.

[sbordet]

It's this part https://github.com/dropwizard/metrics/blob/5ace721a655fd27b271cc8bcbad6565263b21b17/metrics-jetty12/src/main/java/io/dropwizard/metrics/jetty12/InstrumentedQueuedThreadPool.java#L135-L137.

That part is only reporting the current queue size, it has nothing to do with the queue being bounded.

Upgrading 9 to 12 is a big jump, many things have changed, each case is different, and each case may stumble on a different behavior.

Most of the time it is a seamless upgrade, but it really depends on case to case.

We documented most of the major changes starting from here:
https://jetty.org/docs/jetty/12.1/programming-guide/migration/94-to-10.html

[honnix]

Thank you!