Skip to content

Conversation

@kpalang
Copy link
Contributor

@kpalang kpalang commented Oct 21, 2025

Add mode --add-opens directives to the default vmoptions.

@kpalang kpalang requested review from a team, gibson9583, jonbartels, kayyagari and ssrowe and removed request for a team October 21, 2025 15:42
jonbartels
jonbartels previously approved these changes Oct 21, 2025
Copy link
Contributor

@jonbartels jonbartels left a comment

Choose a reason for hiding this comment

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

LGTM

mgaffigan
mgaffigan previously approved these changes Oct 21, 2025
Copy link
Contributor

@mgaffigan mgaffigan left a comment

Choose a reason for hiding this comment

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

I'd like to be removing opens - not adding them, but I don't see a downside this PR.

Copy link
Member

@tonygermano tonygermano left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is the correct thing to do or not. As it is, I think this file only contains the minimum required by the java code in this project.

Do we want to be adding modules just because they could potentially be used in user code or should those just be added to the user's custom options as required?

@mgaffigan
Copy link
Contributor

@kpalang, can you document what is benefitting from these additional opens?

@kpalang
Copy link
Contributor Author

kpalang commented Oct 27, 2025

You two are expecting me to remember why they were needed...

I've now spent an some time looking into it and absolutely cannot find why they were needed...
Closing this PR for the time being.

@kpalang kpalang closed this Oct 27, 2025
@kpalang
Copy link
Contributor Author

kpalang commented Nov 11, 2025

Re-opening as I've found why some of options were needed:

The following two options are required when clicking the Get Operations button in Web Service Sender. As far as I understand the getDefinition() function wants to first save the supposed .wsdl file to disk, and then start reading it. I don't know where the need for java.util.concurrent comes from, but this was complained about first, and java.io appeared only after opening the concurrent module.

--add-opens=java.base/java.util.concurrent=ALL-UNNAMED
--add-opens=java.base/java.io=ALL-UNNAMED

@kpalang kpalang reopened this Nov 11, 2025
@kpalang kpalang dismissed stale reviews from mgaffigan and jonbartels via 81d4228 November 11, 2025 10:58
@kpalang kpalang force-pushed the feature/more_modules branch from ffcbb65 to 81d4228 Compare November 11, 2025 10:58
mgaffigan
mgaffigan previously approved these changes Nov 11, 2025
Copy link
Contributor

@mgaffigan mgaffigan left a comment

Choose a reason for hiding this comment

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

LGTM

@tonygermano
Copy link
Member

Re-opening as I've found why some of options were needed:

The following two options are required when clicking the Get Operations button in Web Service Sender. As far as I understand the getDefinition() function wants to first save the supposed .wsdl file to disk, and then start reading it. I don't know where the need for java.util.concurrent comes from, but this was complained about first, and java.io appeared only after opening the concurrent module.

--add-opens=java.base/java.util.concurrent=ALL-UNNAMED
--add-opens=java.base/java.io=ALL-UNNAMED

So, the interesting thing is that it's only a problem if you give it a bad url, and then it can't serialize the following two exceptions. If you give it a good URL, then it's not a problem, and with a bad URL + these add-opens, you just get a different exception.

com.mirth.connect.model.converters.ObjectXMLSerializer: java.lang.reflect.InaccessibleObjectException: Unable to make field private static final long java.util.concurrent.ExecutionException.serialVersionUID accessible: module java.base does not "opens java.util.concurrent" to unnamed module @b93aad
com.mirth.connect.model.converters.ObjectXMLSerializer: java.lang.reflect.InaccessibleObjectException: Unable to make field static final long java.io.IOException.serialVersionUID accessible: module java.base does not "opens java.io" to unnamed module @54afd745

Copy link
Member

@tonygermano tonygermano left a comment

Choose a reason for hiding this comment

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

Since you turned off the ability for maintainers to make changes, can you apply this patch to fix the order? 😝 You can squash it after applying.

From 8029fc66a8b0f64a7dfc166fea02f4e149eb9018 Mon Sep 17 00:00:00 2001
From: Tony Germano <[email protected]>
Date: Tue, 11 Nov 2025 21:13:20 -0500
Subject: fix order

---
 server/conf/default_modules.vmoptions | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/conf/default_modules.vmoptions b/server/conf/default_modules.vmoptions
index c53651e1f..32a51e085 100644
--- a/server/conf/default_modules.vmoptions
+++ b/server/conf/default_modules.vmoptions
@@ -5,6 +5,7 @@
 --add-modules=java.sql.rowset
 --add-exports=java.base/com.sun.crypto.provider=ALL-UNNAMED
 --add-exports=java.base/sun.security.provider=ALL-UNNAMED
+--add-opens=java.base/java.io=ALL-UNNAMED
 --add-opens=java.base/java.lang=ALL-UNNAMED
 --add-opens=java.base/java.lang.reflect=ALL-UNNAMED
 --add-opens=java.base/java.math=ALL-UNNAMED
@@ -13,11 +14,10 @@
 --add-opens=java.base/java.security.cert=ALL-UNNAMED
 --add-opens=java.base/java.text=ALL-UNNAMED
 --add-opens=java.base/java.util=ALL-UNNAMED
+--add-opens=java.base/java.util.concurrent=ALL-UNNAMED
 --add-opens=java.base/sun.security.pkcs=ALL-UNNAMED
 --add-opens=java.base/sun.security.rsa=ALL-UNNAMED
 --add-opens=java.base/sun.security.x509=ALL-UNNAMED
---add-opens=java.base/java.io=ALL-UNNAMED
---add-opens=java.base/java.util.concurrent=ALL-UNNAMED
 --add-opens=java.desktop/java.awt=ALL-UNNAMED
 --add-opens=java.desktop/java.awt.color=ALL-UNNAMED
 --add-opens=java.desktop/java.awt.font=ALL-UNNAMED
-- 
2.39.5

Copy link
Contributor

@mgaffigan mgaffigan left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants