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

Glassfish 7 Java 21 builds fail in ConnectionPoolTest #24931

Closed
escay opened this issue Apr 22, 2024 · 1 comment · Fixed by #24932
Closed

Glassfish 7 Java 21 builds fail in ConnectionPoolTest #24931

escay opened this issue Apr 22, 2024 · 1 comment · Fixed by #24932

Comments

@escay
Copy link
Contributor

escay commented Apr 22, 2024

Glassfish 7 Java 21 builds fail in ConnectionPoolTest

See also: #24870 (comment)

Example build failures:

https://ci.eclipse.org/glassfish/job/glassfish7_build-and-test_jdk21/283/consoleFull
https://ci.eclipse.org/glassfish/job/glassfish7_build-and-test_jdk21/275/consoleFull

Error:

12:24:49  [INFO] Running com.sun.enterprise.resource.pool.ConnectionPoolTest
12:24:49  [0.686s][info][gc] GC(1) Pause Young (Normal) (G1 Evacuation Pause) 35M->7M(258M) 3.130ms
12:24:49  10:24:49.717848 WARNING                 main              com.sun.enterprise.resource.pool.ConnectionPool.resourceClosed resourceClosed - Expecting state.isBusy(): false, but was true for handle: 122
12:24:49  [ERROR] Tests run: 4, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.097 s <<< FAILURE! -- in com.sun.enterprise.resource.pool.ConnectionPoolTest
12:24:49  [ERROR] com.sun.enterprise.resource.pool.ConnectionPoolTest.basicConnectionPoolMultiThreadedTest -- Time elapsed: 0.024 s <<< FAILURE!
12:24:49  org.opentest4j.AssertionFailedError: expected: <5> but was: <3>
12:24:49  	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
12:24:49  	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
12:24:49  	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
12:24:49  	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
12:24:49  	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
12:24:49  	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
12:24:49  	at com.sun.enterprise.resource.pool.ConnectionPoolTest.assertResourcesSize(ConnectionPoolTest.java:437)
12:24:49  	at com.sun.enterprise.resource.pool.ConnectionPoolTest.basicConnectionPoolMultiThreadedTest(ConnectionPoolTest.java:314)

Analysis:

I added the test about a month ago in commit bdced2d, the test might also be incorrect / my reading / understanding of the code might be incorrect.

Looking at the test itself, I do not think I can prove with the current test code that the maximum of 5 connections in the pool is really reached. Perhaps Jdk21 is quicker with each thread (due to virtual threads enhancements?, although the test is not using virtual threads) which prevents the maximum of the connection pool to be used, and only 3 of 5 are used. Starting 100 tasks with 30 threads in parallel while each one sleeps for sleep(0, 10) nanos as work might not be enough to fill up 5 resources from the pool.

Installed OpenJDK21U-jdk_x64_windows_hotspot_21.0.3_9.zip in windows.

Ran 20 times "mvn test -offline" in module "connectors-runtime", ConnectionPoolTest never failed.
Removed the Thread sleep 20 nanos -> test fails with: org.opentest4j.AssertionFailedError: expected: <5> but was: <3>
Removed the Thread sleep 20 nanos and increased tasks from 100 to 10.000 -> test succeeds

I think the test with line 314 is 'CPU / machine' dependent (I use a 6 year old i7 8700 cpu).

Fixes I can think of:

  1. Remove the test line, this would be ok, because the poolResizeQuantityTest and the basicConnectionPoolTest already test the values while controlling when resources are returned to the pool
  2. Provide some information to the tasks created in runTheTasks to make the first 5 threads occupy the pool for a bit
  3. Do not validate the maximum size is reached, but compare to the set of usedResouceHandles -> rewrite check to test against assertResourcesSize(usedResouceHandles.size());

I would opt for option 1 or 3 to keep it simple, the test is already complicated enough.

escay added a commit to escay/glassfish that referenced this issue Apr 22, 2024
…s' instead of

theoretical maximum 'maxConnectionPoolSize'
dmatej added a commit that referenced this issue Apr 23, 2024
Fix #24931 compare with known size of 'usedResourceHandles' instead of theoretical maximum 'maxConnectionPoolSize'
@escay
Copy link
Contributor Author

escay commented Apr 24, 2024

JDK 21 build is green again
https://ci.eclipse.org/glassfish/job/glassfish7_build-and-test_jdk21/

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