Proposal: Require Java 21

[Ryo_Sangnoir]

Hi all,

Much like we started requiring Java 17 two years ago, we're now considering requiring Java 21 (for example, so that we can use the simd-json library for faster JSON parsing, which may help people with extremely large inventories, like me).

If you're using it, you don't need to make any changes; if not you could download it in preparation. If you're already using Java 21, you may have already noticed a tiny benefit related to GOAWAY handling.

There was some previous discussion at https://kolmafia.us/threads/support-for-java-21-new-lts.29522/

 

[Veracity]

I get a warning on my terminal window with Java 21 on my Mac (macOS Sonoma 14.6.1), but everything seems to work the same as it ever was. (Once in a lifetime, water flowing underground.)

$ java -jar dist/KoLmafia-28139-M.jar 

KoLmafia r28139-M
Build main-981b593-M 21.0.5 (Eclipse Adoptium 21.0.5+11-LTS) Mac OS X aarch64 14.6.1

Currently Running on Mac OS X
Local Directory is /Users/pld/Library/Application Support/KoLmafia
Using Java 21.0.5

2024-11-28 09:30:20.762 java[8918:40868400] *** WARNING: References to Carbon menus are disallowed with AppKit menu system on macOS 14 and newer. Use instances of NSMenu and NSMenuItem directly instead. See https://developer.apple.com/documentation/macos-release-notes/appkit-release-notes-for-macos-14#Menus

 

[heeheehee]

a tiny benefit related to GOAWAY handling.

For what it's worth, I don't think the GOAWAY fix has been backported to Java 21 yet. I think Java 22 might have it, 23 definitely does.

 

[fronobulax]

For what it's worth, I don't think the GOAWAY fix has been backported to Java 21 yet. I think Java 22 might have it, 23 definitely does.

Darn. The "withResponse" test helpers don't work (for me) when the base code uses a resetting connection. It would be easier for me to just deprecate the resetting connection.

 

[heeheehee]

Darn. The "withResponse" test helpers don't work (for me) when the base code uses a resetting connection. It would be easier for me to just deprecate the resetting connection.

https://github.com/openjdk/jdk21u-dev/pull/1020 is the backport to Java 21, FYI.

I'd like to hear more about how it's not working for you, since AFAIK it's not causing problems for our Windows test executions on Github.

 

[fronobulax]

https://github.com/openjdk/jdk21u-dev/pull/1020 is the backport to Java 21, FYI.

I'd like to hear more about how it's not working for you, since AFAIK it's not causing problems for our Windows test executions on Github.

See https://github.com/kolmafia/kolmafia/pull/2523, primarily the last two tests in the file which are driven by function and not a coverage report.

I tried to write a test for setting _faxDataChanged using another test (wiki plurals?) as an example. The example injected an http response. My test did not work, I spent time running the test in the debugger and realized the code I was testing used a resetting request whereas the example used a plain request. I changed the code under test to use a plain request instead of a resetting request and the test worked. I then got somewhat distracted. My working hypothesis is that the existing test framework does not support response injection with a resetting request. A less likely alternative but worth considering is that I wrote the "wrong" test.

I am wondering whether the right solution is to make the resetting request extend the plain request and I may play with that later today.

 

[Ryo_Sangnoir]

withResponse only resets the client on the GenericRequest. The client on FileUtilities is separate and cached, so if any other code uses it first you'd not expect it to be overridden by the fake client created by the tests.

 

[Irrat]

Is it possible that GOAWAY is being suppressed by mafia in some cases and its happening a lot more than we realize?

Eg: https://github.com/kolmafia/kolmafi...ge/kolmafia/request/GenericRequest.java#L1611

Anyways, a few thoughts are

1. We're not actually being thread safe when we reset the client. We don't prevent connections from being made to the previous client, even as we're shutting it down. We also allow it to be reset multiple times if other threads are trying to make connections even as the first caller is waiting for it to be closed. Then of course, we then construct multiple clients.
But that may be rare, I noticed it when I was spamming http connections to try replicate GOAWAY that it was recreating the httpclient multiple times. But I also have a 300ms ping, and there was a lot of connections.

2. Is it possible that we're not shutting it down properly. I mean, that the underlying executor service may still be running and ports still open at the time it closes and starts waiting to be garbage collected. Then the new client, takes over the existing resources? Eg, reusing an opened connection. I don't really understand the mechanics of this myself, it might not even be a thing at all.
My thoughts about that is when I was googling around, I see references to people accessing the internals of httpclient and closing them, then invoking the garbage collector. It might also be specific to their issue and not here.

So just my thoughts. I'm not sure how relevant they are, and how much of it is not a thing.

 

[fronobulax]

Tests that use the resetting client require infrastructure that I just have not convinced myself I understand enough to create. Since the resetting client was created to address a problem that is, or will be fixed, in Java 21 I have been toying with ripping it out. But if GOAWAY is still a problem with the current code and Java 21 perhaps a separate discussion elsewhere might be appropriated. Whether it gets ripped out or not the thread safe issues are worthy of study if not fixes, IMO.

 

[heeheehee]

a problem that is, or will be fixed, in Java 21

"will" is the operative word, but it's not entirely obvious when. It'll definitely be fixed when we adopt Java 25 after September, and there is a clear desire to backport this fix to Java 21, but the PR has not yet landed.

 

[Irrat]

Small suggestion for when we know the next Java version bump will be happening, have a warning label plastered on the mafia login screen about the upcoming update perhaps?

Along with a red message in CLI after a player logs in, just incase they automated their login process.

Heck, add it into Events so they see it in the relay screen!

The java update came as a surprise to a lot of people, and I can't really blame them if they don't follow mafia at all.

Edit: Also to add, it appears that none of my local changes has prevented GOAWAY from appearing. So sounds like the future update / PR are the only reliable changes without some extremity.