Fixes issue 24868 Clean code: fix unlikely argument and remove some unused variables. #24870

escay · 2024-03-21T11:38:22Z

ManagedBeanManagerImpl and JdbcConnPoolAppStatsProvider need specific review.
I did not add a nullcheck for SimpleJndiName in JdbcConnPoolAppStatsProvider, since the constructor already validates it.

## Unlikely argument type SimpleJndiName for remove(Object) on a Map<String,NamingObjectProxy>	ManagedBeanManagerImpl.java	/container-common/src/main/java/com/sun/enterprise/container/common/impl/managedbean	line 338	Java Problem
## Unlikely argument type for equals(): SimpleJndiName seems to be unrelated to String	JdbcConnPoolAppStatsProvider.java	/jdbc-runtime/src/main/java/org/glassfish/jdbc/pool/monitor	line 90	Java Problem
## Unlikely argument type for equals(): SimpleJndiName seems to be unrelated to String	JdbcConnPoolAppStatsProvider.java	/jdbc-runtime/src/main/java/org/glassfish/jdbc/pool/monitor	line 109	Java Problem
## Unlikely argument type for equals(): SimpleJndiName seems to be unrelated to String	JdbcConnPoolAppStatsProvider.java	/jdbc-runtime/src/main/java/org/glassfish/jdbc/pool/monitor	line 124	Java Problem
## Unlikely argument type for equals(): SimpleJndiName seems to be unrelated to String	JdbcConnPoolAppStatsProvider.java	/jdbc-runtime/src/main/java/org/glassfish/jdbc/pool/monitor	line 136	Java Problem

Full list of items is listed in issue #24868

unused variables. ManagedBeanManagerImpl and JdbcConnPoolAppStatsProvider need specific review.

unused variables. ManagedBeanManagerImpl and JdbcConnPoolAppStatsProvider need specific review. (Remove unused imports)

escay · 2024-03-21T12:20:48Z

I will have a look at the

[2024-03-21T11:49:02.051Z] [INFO] There are 2 errors reported by Checkstyle 10.12.4 with org/glassfish/qa/config/checkstyle/checkstyle.xml ruleset.
[2024-03-21T11:49:02.051Z] [ERROR] src/main/java/org/glassfish/appclient/client/acc/HTTPInputArchive.java:[125] (regexp) RegexpSingleline: Line has trailing spaces.
[2024-03-21T11:49:02.051Z] [ERROR] src/main/java/org/glassfish/appclient/client/acc/HTTPInputArchive.java:[130] (regexp) RegexpSingleline: Line has trailing spaces.

build failure.

Note to myself: mvn checkstyle:check -Pqa for a quick test if build mvn clean install -Pfastest -T4C succeeded.

unused variables. ManagedBeanManagerImpl and JdbcConnPoolAppStatsProvider need specific review. (Fix checkstyle 'Line has trailing spaces')

escay · 2024-03-21T14:44:51Z

I probably made something in 'appserv-tests'

[2024-03-21T13:02:35.435Z]      [exec] input file=/home/jenkins/agent/workspace/_test-using-jenkinsfile_PR-24870/appserver/tests/appserv-tests/test_resultsValid.xml
[2024-03-21T13:02:35.435Z]      [exec] 
[2024-03-21T13:02:35.435Z]      [exec] ************************
[2024-03-21T13:02:35.435Z]      [exec] PASSED=   31
[2024-03-21T13:02:35.435Z]      [exec] ------------  =========
[2024-03-21T13:02:35.435Z]      [exec] FAILED=   1

fail. I will look into it.

escay · 2024-03-22T13:30:08Z

Build server tests now succeed (thank you for starting a new build manually, was the cause the "java.net.BindException: Address already in use"?) And I also ran the ant tests locally and they succeed:

     [exec] input file=/mnt/d/gf-escay/glassfish/appserver/tests/appserv-tests/test_resultsValid.xml
     [exec] ************************
     [exec] PASSED=   32
     [exec] ------------  =========
     [exec] FAILED=   0

pzygielo · 2024-03-22T14:28:16Z

Commit comment and PR title suggest that this fixes some issue.

GH provides https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue - with specific syntax for that to happen automatically.

dmatej

Approved, except that ordering of imports, but as we still don't have checkstyle rule enabled for that I can accept that even as it is.

dmatej · 2024-03-23T22:05:24Z

...c/main/java/com/sun/enterprise/container/common/impl/managedbean/ManagedBeanManagerImpl.java

@@ -335,7 +335,7 @@ public void unloadManagedBeans(Application app) {
                            + " with jndi name " + jndiName, ne);
                    }
                } else {
-                    appClientManagedBeans.remove(jndiName);
+                    appClientManagedBeans.remove(jndiName.toString());


Wow, good catch!

dmatej · 2024-03-23T22:06:39Z

...ime/src/main/java/com/sun/enterprise/resource/deployer/JMSDestinationDefinitionDeployer.java

@@ -43,7 +35,13 @@
 import org.jvnet.hk2.config.TransactionFailure;
 import org.jvnet.hk2.config.types.Property;

-import static com.sun.appserv.connectors.internal.api.ConnectorsUtil.deriveResourceName;
+import com.sun.appserv.connectors.internal.api.ConnectorConstants;


Imports should be ordered by alphabet. Static imports should be first or last, but together, that is ok.

Sorry I assumed the ee4j Eclipse formatter took care of the order. I will minimise import statement changes in the future.

dmatej · 2024-03-23T22:11:44Z

appserver/web/web-core/src/main/java/org/apache/catalina/startup/ExpandWar.java

-                try {
-                    ic = (new FileInputStream(fileSrc)).getChannel();
-                    oc = (new FileOutputStream(fileDest)).getChannel();
+                try (FileChannel ic = (new FileInputStream(fileSrc)).getChannel();


I'm not sure now, is closing the channel enough to close the input stream? Is it possible that GC would close the input stream before the channel (no reference to the IS)?

I am not sure. I did compare it to the latest real catalina Expandwar code. They used the same approach, so I assumed it was safe. Note that latest ExpandWar in catalina has some other fixes that are not in this fork.
https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/startup/ExpandWar.java

dmatej · 2024-04-21T10:42:25Z

Is it possible you introduced a new error somehow - sometimes Jenkins fails this test on JDK21, that did not happen before:

org.opentest4j.AssertionFailedError: expected: <5> but was: <3>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
	at com.sun.enterprise.resource.pool.ConnectionPoolTest.assertResourcesSize(ConnectionPoolTest.java:437)
	at com.sun.enterprise.resource.pool.ConnectionPoolTest.basicConnectionPoolMultiThreadedTest(ConnectionPoolTest.java:314)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

It is quite often but not well reproducible, seems like some race condition. Could be caused also by some other change, but this is the closest one.

escay · 2024-04-21T16:57:06Z

I added the test about a month ago, the test might also be incorrect / my reading / understanding of the code might be incorrect.
I have not tested with JDK21 myself, but I can try it out tomorrow.

If it is needed line ConnectionPoolTest.java:314 could be disabled / removed.

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.

escay · 2024-04-22T07:56:39Z

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:

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
Provide some information to the tasks created in runTheTasks to make the first 5 threads occupy the pool for a bit
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 · 2024-04-22T08:35:00Z

Created issue #24931 to improve the test, creating a pull request.

escay added 2 commits March 21, 2024 12:11

Fixes issue 24868 Clean code: fix unlikely argument and remove some

3cc5c75

unused variables. ManagedBeanManagerImpl and JdbcConnPoolAppStatsProvider need specific review.

Fixes issue 24868 Clean code: fix unlikely argument and remove some

88459ae

unused variables. ManagedBeanManagerImpl and JdbcConnPoolAppStatsProvider need specific review. (Remove unused imports)

Fixes issue 24868 Clean code: fix unlikely argument and remove some

bbd6bf7

unused variables. ManagedBeanManagerImpl and JdbcConnPoolAppStatsProvider need specific review. (Fix checkstyle 'Line has trailing spaces')

dmatej approved these changes Mar 23, 2024

View reviewed changes

dmatej added this to the 7.0.14 milestone Mar 23, 2024

dmatej added bug Something isn't working code cleaning labels Mar 23, 2024

arjantijms approved these changes Mar 24, 2024

View reviewed changes

dmatej merged commit 0fce188 into eclipse-ee4j:master Mar 26, 2024
2 checks passed

escay deleted the issue_24868b branch March 26, 2024 19:26

escay mentioned this pull request Apr 22, 2024

Glassfish 7 Java 21 builds fail in ConnectionPoolTest #24931

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issue 24868 Clean code: fix unlikely argument and remove some unused variables. #24870

Fixes issue 24868 Clean code: fix unlikely argument and remove some unused variables. #24870

escay commented Mar 21, 2024 •

edited

escay commented Mar 21, 2024 •

edited

escay commented Mar 21, 2024 •

edited

escay commented Mar 22, 2024 •

edited

pzygielo commented Mar 22, 2024

dmatej left a comment

dmatej Mar 23, 2024

dmatej Mar 23, 2024

escay Mar 23, 2024

dmatej Mar 23, 2024

escay Mar 23, 2024

dmatej commented Apr 21, 2024

escay commented Apr 21, 2024 •

edited

escay commented Apr 22, 2024 •

edited

escay commented Apr 22, 2024

Fixes issue 24868 Clean code: fix unlikely argument and remove some unused variables. #24870

Fixes issue 24868 Clean code: fix unlikely argument and remove some unused variables. #24870

Conversation

escay commented Mar 21, 2024 • edited

escay commented Mar 21, 2024 • edited

escay commented Mar 21, 2024 • edited

escay commented Mar 22, 2024 • edited

pzygielo commented Mar 22, 2024

dmatej left a comment

Choose a reason for hiding this comment

dmatej Mar 23, 2024

Choose a reason for hiding this comment

dmatej Mar 23, 2024

Choose a reason for hiding this comment

escay Mar 23, 2024

Choose a reason for hiding this comment

dmatej Mar 23, 2024

Choose a reason for hiding this comment

escay Mar 23, 2024

Choose a reason for hiding this comment

dmatej commented Apr 21, 2024

escay commented Apr 21, 2024 • edited

escay commented Apr 22, 2024 • edited

escay commented Apr 22, 2024

escay commented Mar 21, 2024 •

edited

escay commented Mar 21, 2024 •

edited

escay commented Mar 21, 2024 •

edited

escay commented Mar 22, 2024 •

edited

escay commented Apr 21, 2024 •

edited

escay commented Apr 22, 2024 •

edited