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.

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

HikariPool - fix major bug with usage of com.zaxxer.hikari.blockUntilFilled in JDK 11 and later #1605

Conversation

sw-pachanady
Copy link
Contributor

@sw-pachanady sw-pachanady commented Jun 22, 2020

Due to a change in JDK11 in ThreadPoolExecutor setMaximumPoolSize needs to be done before setCorePoolSize, the current implementation throws IllegalArgumentException and hence prefilling of connections by setting com.zaxxer.hikari.blockUntilFilled is broken.

HikariPool -line 133 - addConnectionExecutor initializes the ThreadPoolExecutor with 1 core and 1 max thread.

Then we get an IllegalArgumentException in line 141while trying to set setCorePoolSize due to check

NOTE: This bug and PR was submitted by some other developers in the past I think, but it's still not fixed.

if (corePoolSize < 0 || maximumPoolSize < corePoolSize)
throw new IllegalArgumentException();

1 < some number of core pools make it throw IllegalArgumentException

Hence setting "com.zaxxer.hikari.blockUntilFilled " to true and trying to prefill connections fails.
This feature was working fine in JDK 8. Kindly review and merge this pull request.

…mumPoolSize needs to be done before setCorePoolSize, the current implementation throws IllegalArgumentException and hence prefilling of connections by setting com.zaxxer.hikari.blockUntilFilled is broken.
@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #1605 into dev will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                dev    #1605   +/-   ##
=========================================
  Coverage     70.11%   70.11%           
  Complexity      561      561           
=========================================
  Files            26       26           
  Lines          2118     2118           
  Branches        296      296           
=========================================
  Hits           1485     1485           
  Misses          487      487           
  Partials        146      146           
Impacted Files Coverage Δ Complexity Δ
...c/main/java/com/zaxxer/hikari/pool/HikariPool.java 81.00% <0.00%> (ø) 59.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a28f6e...9c52fc5. Read the comment docs.

@johnou
Copy link
Contributor

johnou commented Jan 7, 2021

@brettwooldridge this looks like a blocker for those wishing to adopt jdk 11.

@brettwooldridge brettwooldridge merged commit 00f5f20 into brettwooldridge:dev Jan 11, 2021
@pentlander
Copy link

pentlander commented Feb 9, 2021

I don't think this is correct, addConnectionExecutor.setCorePoolSize(1); should still occur before addConnectionExecutor.setMaximumPoolSize(1); otherwise you still run into issues. I'm still getting an exception when running 4.0.1 this time on https://github.com/sw-pachanady/HikariCP/blob/9c52fc556094f4d1bc3835bcb54a50cea3d48e97/src/main/java/com/zaxxer/hikari/pool/HikariPool.java#L149

@johnou
Copy link
Contributor

johnou commented Feb 9, 2021

@pentlander please provide the stracktrace.

@pentlander
Copy link

pentlander commented Feb 9, 2021

Well logically once line 149 is reached, the core size and max size are at least 16. Once you try to shrink the max size to 1, this line gets run and fails because the core size is still 16. I'll attach a stacktrace tomorrow morning.

@pentlander
Copy link

Using Hikari 4.0.1 with OpenJDK 15, with "com.zaxxer.hikari.blockUntilFilled" set to true and initialization fail timeout at 10_000:

Uncaught exception in thread main
java.lang.IllegalArgumentException: null
	at java.base/java.util.concurrent.ThreadPoolExecutor.setMaximumPoolSize(Unknown Source)
	at com.zaxxer.hikari.pool.HikariPool.<init>(HikariPool.java:149)
	at com.zaxxer.hikari.HikariDataSource.<init>(HikariDataSource.java:81)
        ...

@johnou
Copy link
Contributor

johnou commented Feb 10, 2021

wow looks like we need some conditional checks when setting the value to avoid the iae.

    public void setCorePoolSize(int corePoolSize) {
        if (corePoolSize < 0 || maximumPoolSize < corePoolSize)
            throw new IllegalArgumentException();

    public void setMaximumPoolSize(int maximumPoolSize) {
        if (maximumPoolSize <= 0 || maximumPoolSize < corePoolSize)
            throw new IllegalArgumentException();
        this.maximumPoolSize = maximumPoolSize;

@pentlander
Copy link

It should be fine if 149 and 150 are flipped back to their original order, i.e.

addConnectionExecutor.setCorePoolSize(1);
addConnectionExecutor.setMaximumPoolSize(1);

@brettwooldridge
Copy link
Owner

Wait. Are we saying that in JDK 11 we need to call setMaximumPoolSize() before setCorePoolSize(), but in JDK 15 we need to call setCorePoolSize() before setMaximumPoolSize()?

@pentlander
Copy link

pentlander commented Feb 12, 2021

No, I'm saying that the new code increasing the pool size is correct here because it increases the max size to 16 before increasing the core size, avoiding this condition:

public void setCorePoolSize(int corePoolSize) {
        if (corePoolSize < 0 || maximumPoolSize < corePoolSize)
            throw new IllegalArgumentException();

The new code is incorrect here because it is reducing the max pool size to 1 while the core pool size is still 16, thus triggering:

public void setMaximumPoolSize(int maximumPoolSize) {
        if (maximumPoolSize <= 0 || maximumPoolSize < corePoolSize)
            throw new IllegalArgumentException();

If the latter code is reverted to this it should work, since the the core size will always be less than the max size:

addConnectionExecutor.setCorePoolSize(1);
addConnectionExecutor.setMaximumPoolSize(1);

I don't know why the author of the pull request didn't run into this issue, the JDK11 checks are the same as the JDK15 ones: https://github.com/openjdk/jdk11/blob/37115c8ea4aff13a8148ee2b8832b20888a5d880/src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java#L1658

brettwooldridge added a commit that referenced this pull request Feb 13, 2021
…or pool sizes

back to defaults, the core pool size must be changed before the maximum pool size.
@brettwooldridge
Copy link
Owner

Fixed and released as v4.0.2.

@pentlander
Copy link

Fixed and released as v4.0.2.

I validated it's working with JDK15. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants