Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8347491: IllegalArgumentationException thrown by ThreadPoolExecutor doesn't have a useful message #23050

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+247 −100
Open

8347491: IllegalArgumentationException thrown by ThreadPoolExecutor doesn't have a useful message #23050

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
219efbe
8347491: Add detail error messages when ThreadPoolExecutor's paramete…
He-Pin Jan 11, 2025
6bd4ec7
chore: add test cases
He-Pin Jan 13, 2025
173437f
chore: tweak the error messages
He-Pin Jan 13, 2025
1de900f
chore: add messages for NPE
He-Pin Jan 13, 2025
c1b4846
.
He-Pin Jan 14, 2025
dfc2cee
chore: update tests
He-Pin Feb 24, 2025
135a2d0
.
He-Pin Mar 2, 2025
1e098b2
.
He-Pin Mar 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java
View file
Original file line number Diff line number Diff line change
@@ -1257,13 +1257,13 @@ public ThreadPoolExecutor(int corePoolSize,
ThreadFactory threadFactory,
RejectedExecutionHandler handler) {
if (corePoolSize < 0) {
throw new IllegalArgumentException("corePoolSize must be non-negative, but got " + corePoolSize);
throw new IllegalArgumentException("corePoolSize must be non-negative");
} else if (maximumPoolSize <= 0) {
throw new IllegalArgumentException("maximumPoolSize must be positive, but got " + maximumPoolSize);
throw new IllegalArgumentException("maximumPoolSize must be positive");
} else if (maximumPoolSize < corePoolSize) {
throw new IllegalArgumentException("maximumPoolSize must be greater than or equal to corePoolSize , " + "but got maximumPoolSize:" + maximumPoolSize + " ,corePoolSize:" + corePoolSize);
throw new IllegalArgumentException("maximumPoolSize must be greater than or equal to corePoolSize");
} else if (keepAliveTime < 0) {
throw new IllegalArgumentException("keepAliveTime must be non-negative, but got " + keepAliveTime);
throw new IllegalArgumentException("keepAliveTime must be non-negative");
}
Objects.requireNonNull(unit, "unit");
Objects.requireNonNull(workQueue, "workQueue");
@@ -1507,10 +1507,9 @@ public RejectedExecutionHandler getRejectedExecutionHandler() {
*/
public void setCorePoolSize(int corePoolSize) {
if (corePoolSize < 0) {
throw new IllegalArgumentException("corePoolSize must be non-negative, but got " + corePoolSize);
throw new IllegalArgumentException("corePoolSize must be non-negative");
} else if (corePoolSize > maximumPoolSize) {
throw new IllegalArgumentException("corePoolSize must be less than or equal to maximumPoolSize, " +
"but got maximumPoolSize:" + maximumPoolSize + " corePoolSize :" + corePoolSize);
throw new IllegalArgumentException("corePoolSize must be less than or equal to maximumPoolSize");
}
int delta = corePoolSize - this.corePoolSize;
this.corePoolSize = corePoolSize;
@@ -1636,10 +1635,9 @@ public void allowCoreThreadTimeOut(boolean value) {
*/
public void setMaximumPoolSize(int maximumPoolSize) {
if (maximumPoolSize <= 0) {
throw new IllegalArgumentException("maximumPoolSize must be positive, but got " + maximumPoolSize);
throw new IllegalArgumentException("maximumPoolSize must be positive");
} else if (maximumPoolSize < corePoolSize) {
throw new IllegalArgumentException("maximumPoolSize must be greater than or equal to corePoolSize , " +
"but got maximumPoolSize:" + maximumPoolSize + " corePoolSize :" + corePoolSize);
throw new IllegalArgumentException("maximumPoolSize must be greater than or equal to corePoolSize");
}
this.maximumPoolSize = maximumPoolSize;
if (workerCountOf(ctl.get()) > maximumPoolSize)
@@ -1674,9 +1672,10 @@ public int getMaximumPoolSize() {
*/
public void setKeepAliveTime(long time, TimeUnit unit) {
if (time < 0)
throw new IllegalArgumentException("time must be non-negative, but got " + time);
throw new IllegalArgumentException("time must be non-negative");
if (time == 0 && allowsCoreThreadTimeOut())
throw new IllegalArgumentException("Core threads must have nonzero keep alive times");
Objects.requireNonNull(unit, "unit");
long keepAliveTime = unit.toNanos(time);
long delta = keepAliveTime - this.keepAliveTime;
this.keepAliveTime = keepAliveTime;
103 changes: 49 additions & 54 deletions test/jdk/java/util/concurrent/tck/ThreadPoolExecutorTest.java
View file
Original file line number Diff line number Diff line change
@@ -41,22 +41,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.Callable;
import java.util.concurrent.CancellationException;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.concurrent.FutureTask;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.RejectedExecutionHandler;
import java.util.concurrent.SynchronousQueue;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.*;
import java.util.concurrent.ThreadPoolExecutor.AbortPolicy;
import java.util.concurrent.ThreadPoolExecutor.CallerRunsPolicy;
import java.util.concurrent.ThreadPoolExecutor.DiscardPolicy;
@@ -743,7 +728,7 @@ public void testConstructor1() {
new ArrayBlockingQueue<Runnable>(10));
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("corePoolSize must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("corePoolSize must be non-negative");
}
}

@@ -756,7 +741,7 @@ public void testConstructor2() {
new ArrayBlockingQueue<Runnable>(10));
shouldThrow();
} catch (IllegalArgumentException success) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could test the actual message here.

Assert.assertEquals("maximumPoolSize must be positive, but got -1", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive");
}
}

@@ -769,7 +754,7 @@ public void testConstructor3() {
new ArrayBlockingQueue<Runnable>(10));
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("maximumPoolSize must be positive, but got 0", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive", success.getMessage());
}
}

@@ -782,7 +767,7 @@ public void testConstructor4() {
new ArrayBlockingQueue<Runnable>(10));
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("keepAliveTime must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("keepAliveTime must be non-negative", success.getMessage());
}
}

@@ -796,8 +781,7 @@ public void testConstructor5() {
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals(
"maximumPoolSize must be greater than or equal to corePoolSize ,but got maximumPoolSize:1 ," +
"corePoolSize:2",
"maximumPoolSize must be greater than or equal to corePoolSize",
success.getMessage()
);
}
@@ -826,7 +810,7 @@ public void testConstructor6() {
new SimpleThreadFactory());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("corePoolSize must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("corePoolSize must be non-negative", success.getMessage());
}
}

@@ -840,7 +824,7 @@ public void testConstructor7() {
new SimpleThreadFactory());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("maximumPoolSize must be positive, but got -1", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive", success.getMessage());
}
}

@@ -854,7 +838,7 @@ public void testConstructor8() {
new SimpleThreadFactory());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("maximumPoolSize must be positive, but got 0", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive", success.getMessage());
}
}

@@ -868,7 +852,7 @@ public void testConstructor9() {
new SimpleThreadFactory());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("keepAliveTime must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("keepAliveTime must be non-negative", success.getMessage());
}
}

@@ -883,8 +867,7 @@ public void testConstructor10() {
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals(
"maximumPoolSize must be greater than or equal to corePoolSize ,but got maximumPoolSize:1 ," +
"corePoolSize:2",
"maximumPoolSize must be greater than or equal to corePoolSize",
success.getMessage()
);
}
@@ -928,7 +911,7 @@ public void testConstructor11() {
new NoOpREHandler());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("corePoolSize must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("corePoolSize must be non-negative", success.getMessage());
}
}

@@ -942,7 +925,7 @@ public void testConstructor12() {
new NoOpREHandler());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("maximumPoolSize must be positive, but got -1", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive", success.getMessage());
}
}

@@ -956,7 +939,7 @@ public void testConstructor13() {
new NoOpREHandler());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("maximumPoolSize must be positive, but got 0", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive", success.getMessage());
}
}

@@ -970,7 +953,7 @@ public void testConstructor14() {
new NoOpREHandler());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("keepAliveTime must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("keepAliveTime must be non-negative", success.getMessage());
}
}

@@ -985,8 +968,7 @@ public void testConstructor15() {
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals(
"maximumPoolSize must be greater than or equal to corePoolSize ,but got maximumPoolSize:1 ," +
"corePoolSize:2",
"maximumPoolSize must be greater than or equal to corePoolSize",
success.getMessage()
);
}
@@ -1031,7 +1013,7 @@ public void testConstructor16() {
new NoOpREHandler());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("corePoolSize must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("corePoolSize must be non-negative", success.getMessage());
}
}

@@ -1046,7 +1028,7 @@ public void testConstructor17() {
new NoOpREHandler());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("maximumPoolSize must be positive, but got -1", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive", success.getMessage());
}
}

@@ -1061,7 +1043,7 @@ public void testConstructor18() {
new NoOpREHandler());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("maximumPoolSize must be positive, but got 0", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive", success.getMessage());
}
}

@@ -1076,7 +1058,7 @@ public void testConstructor19() {
new NoOpREHandler());
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("keepAliveTime must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("keepAliveTime must be non-negative", success.getMessage());
}
}

@@ -1092,8 +1074,7 @@ public void testConstructor20() {
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals(
"maximumPoolSize must be greater than or equal to corePoolSize ,but got maximumPoolSize:1 ," +
"corePoolSize:2",
"maximumPoolSize must be greater than or equal to corePoolSize",
success.getMessage()
);
}
@@ -1155,7 +1136,7 @@ public void testConstructorNullPointerException9() {
new NoOpREHandler());
shouldThrow();
} catch (NullPointerException success) {
Assert.assertEquals("threadFactory", success.getMessage());
Assert.assertEquals("unit", success.getMessage());
}
}

@@ -1321,7 +1302,7 @@ public void testCorePoolSizeIllegalArgumentException() {
p.setCorePoolSize(-1);
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("corePoolSize must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("corePoolSize must be non-negative", success.getMessage());
}
}
}
@@ -1341,8 +1322,7 @@ public void testMaximumPoolSizeIllegalArgumentException() {
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals(
"maximumPoolSize must be greater than or equal to corePoolSize ,but got maximumPoolSize:1 ," +
"corePoolSize:2",
"maximumPoolSize must be greater than or equal to corePoolSize",
success.getMessage()
);
}
@@ -1363,7 +1343,7 @@ public void testMaximumPoolSizeIllegalArgumentException2() {
p.setMaximumPoolSize(-1);
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("maximumPoolSize must be positive, but got -1", success.getMessage());
Assert.assertEquals("maximumPoolSize must be positive");
}
}
}
@@ -1381,27 +1361,23 @@ public void testPoolSizeInvariants() {
for (int s = 1; s < 5; s++) {
p.setMaximumPoolSize(s);
p.setCorePoolSize(s);
int s1 = s - 1;
try {
p.setMaximumPoolSize(s1);
p.setMaximumPoolSize(s - 1);
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals(
"maximumPoolSize must be greater than or equal to corePoolSize ,but got maximumPoolSize:" +
s1 + " ,corePoolSize:" + s,
"maximumPoolSize must be greater than or equal to corePoolSize",
success.getMessage()
);
}
assertEquals(s, p.getCorePoolSize());
assertEquals(s, p.getMaximumPoolSize());
int s2 = s + 1;
try {
p.setCorePoolSize(s2);
p.setCorePoolSize(s + 1);
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals(
"maximumPoolSize must be greater than or equal to corePoolSize ,but got maximumPoolSize:" +
s + " ,corePoolSize:" + s2,
"maximumPoolSize must be greater than or equal to corePoolSize",
success.getMessage()
);
}
@@ -1425,7 +1401,26 @@ public void testKeepAliveTimeIllegalArgumentException() {
p.setKeepAliveTime(-1, MILLISECONDS);
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("keepAliveTime must be non-negative, but got -1", success.getMessage());
Assert.assertEquals("keepAliveTime must be non-negative");
}
}
}

/**
* setKeepAliveTime throws IllegalArgumentException
* when given a null unit
*/
public void testKeepAliveTimeIllegalArgumentException() {
final ThreadPoolExecutor p =
new ThreadPoolExecutor(2, 3,
LONG_DELAY_MS, MILLISECONDS,
new ArrayBlockingQueue<Runnable>(10));
try (PoolCleaner cleaner = cleaner(p)) {
try {
p.setKeepAliveTime(1, (TimeUnit) null);
shouldThrow();
} catch (IllegalArgumentException success) {
Assert.assertEquals("unit", success.getMessage());
}
}
}