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

Fix java 21 build #421

Merged
merged 6 commits into from
Oct 20, 2023
Merged

Fix java 21 build #421

merged 6 commits into from
Oct 20, 2023

Conversation

wetted
Copy link
Contributor

@wetted wetted commented Oct 18, 2023

Supersedes #415 to stop micronaut-build force pushing it all the time

@timyates
Copy link
Contributor

@sdelamo This change seems to be where this error began: e9a0b15

This reverts commit e9a0b15.
@timyates
Copy link
Contributor

I've reverted that commit. An import wasn't required (as the class is in the same package) and the commit changed the class that was returned to the wrong one

@timyates
Copy link
Contributor

This test seems to hang under Java 21 🤔

@melix any ideas?

@melix
Copy link
Collaborator

melix commented Oct 19, 2023

No I don't.

@timyates
Copy link
Contributor

Found it... PR incoming... Lock issue I think

@timyates
Copy link
Contributor

timyates commented Oct 19, 2023

Ok, so the issue was that under Java 21 the controller switches to using virtual executors.

Inside TestContainers we use a re-entrant lock and this all worked as it was effectively the same thread making all the requests

10:44:44.911 [io-executor-thread-1] TRACE i.m.t.testcontainers.TestContainers - Unlocked for getOrCreate
10:44:44.911 [io-executor-thread-1] DEBUG i.m.t.server.TestResourcesController - Attempt to resolve kafka.bootstrap.servers with resolver class io.micronaut.testresources.kafka.KafkaTestResourceProvider, properties {} and test resources configuration {} : PLAINTEXT://127.0.0.1:50708
10:44:44.920 [io-executor-thread-1] TRACE i.m.t.testcontainers.TestContainers - Locked for listByScope
10:44:44.920 [io-executor-thread-1] TRACE i.m.t.testcontainers.TestContainers - Unlocked for listByScope
10:44:44.927 [io-executor-thread-1] DEBUG i.m.t.server.TestResourcesController - Closing all test resources
10:44:44.927 [io-executor-thread-1] TRACE i.m.t.testcontainers.TestContainers - Locked for closeAll
10:44:45.076 [io-executor-thread-1] TRACE i.m.t.testcontainers.TestContainers - Unlocked for closeAll
10:44:45.085 [io-executor-thread-1] TRACE i.m.t.testcontainers.TestContainers - Locked for listByScope
10:44:45.085 [io-executor-thread-1] TRACE i.m.t.testcontainers.TestContainers - Unlocked for listByScope

But with Java 21, we're seeing a new thread per request, and this is causing issues as re-entrant lock will not let these other thread in if it hasn't been unlocked

10:50:33.676 [virtual-executor4] TRACE i.m.t.testcontainers.TestContainers - Locked for listByScope
10:50:33.676 [virtual-executor4] TRACE i.m.t.testcontainers.TestContainers - Unlocked for listByScope
10:50:33.693 [virtual-executor5] DEBUG i.m.t.server.TestResourcesController - Closing all test resources
10:50:33.693 [virtual-executor5] TRACE i.m.t.testcontainers.TestContainers - Locked for closeAll
10:50:33.830 [virtual-executor5] TRACE i.m.t.testcontainers.TestContainers - Unlocked for closeAll
10:50:33.833 [virtual-executor6] TRACE i.m.t.testcontainers.TestContainers - Locked for listByScope
10:50:33.834 [virtual-executor6] TRACE i.m.t.testcontainers.TestContainers - Unlocked for listByScope

I think this is fixed by ensuring all LOCK/UNLOCK couplets are in a try...finally

We could also force java 17 behavior by using @ExecuteOn(IO) in the controller methods...

But I think checking the locks are unlocked is the more correct behavior...

@timyates timyates requested review from melix and sdelamo October 19, 2023 09:54
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@melix melix merged commit cd86849 into master Oct 20, 2023
@melix melix deleted the fix-java-21-build branch October 20, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants