google / error-prone

Catch common Java mistakes as compile-time errors
https://errorprone.info
Apache License 2.0
6.82k stars 740 forks source link

New rule: Buggy ThreadPoolExecutor initializations #1510

Open JanecekPetr opened 4 years ago

JanecekPetr commented 4 years ago

Description of the feature request:

Imagine this:

executor = new ThreadPoolExecutor(
        1, 4,
        1, TimeUnit.MINUTES,
        new LinkedBlockingQueue<>()
);

This appears to try to create a TPE with a single thread that scales up to four threads when tasks come in. So what will happen if this executor gets 5 requests at the same time?

Only a single thread will be created, four tasks will wait in a queue. In fact, this executor will never have more than 1 thread.

See ThreadPoolExecutor, section "Queueing".

The insight here is that after the core threads are running, the executor prefers to enqueue tasks instead of starting extra threads. For an unbounded queue, that means that only core threads will ever run.

A bounded queue with a limit high enough (Millions? Maybe. Definitely Integer.MAX_VALUE, though.) will effectively behave the same.

The rule should suggest to either use coreThreads == maxThreads (optionally with executor.allowCoreThreadTimeOut(true)), or use an effectively bounded queue (or simply suggest the Executors class).

Feature requests: what underlying problem are you trying to solve with this feature?

An actual bug pattern that occasionally pops up, is hard to find and debug.

What version of Error Prone are you using?

2.3.4

Have you found anything relevant by searching the web?

This is known behaviour of TPE that was discussed a few times at the concurrency-interest mailing list. The behaviour makes sense, but is initially unintuitive and error-prone.

JanecekPetr commented 4 years ago

I'm a little unsure what's going on now.

I know from Guava that external contributions are rarely accepted as most things already exist in Google and only things that are internally considered safe, resolved and stable are getting open-sourced. Is this a similar situation?

Or are you guys simply super-busy right now and would consider accepting a contribution of the above described check? I'm happy to give it a go if there's non-zero chance of discussion / approval :)

Or maybe it's too soon to write code and more research on similar / related issues in Executor usage is needed first?