Fix potential double acquisition in SingleLock

[leonard84]

SingleLock uses a ForkJoinPool.ManagedBlocker for efficient blocking in a fork-join pool. In the isReleasable() method it calls tryLock() without remembering the outcome. The block method also unconditionally called lockInterruptibly() even if the lock was already acquired. The problem that can arise is that the lock is acquired multiple times, but only released once.

CompositeLockManagedBlocker is also updated, so that block() checks for acquired before locking.

Overview

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[leonard84]

Spock started to experience strange deadlocks the thread dumps point to the locking infrastructure being the culprit.

I checked the JavaDoc of ManagedBlocker again, and it was updated, or we didn't implement it correctly back then.

class ManagedLocker implements ManagedBlocker {
  final ReentrantLock lock;
  boolean hasLock = false;
  ManagedLocker(ReentrantLock lock) { this.lock = lock; }
  public boolean block() {
    if (!hasLock)
      lock.lock();
    return true;
  }
  public boolean isReleasable() {
    return hasLock || (hasLock = lock.tryLock());
  }
}

The JavaDoc suggests that this isn't the issue, although the example implementation guards against repeated locking, so I think we should follow its guidance.

Toward this end, implementations of method isReleasable must be amenable to repeated invocation. Neither method is invoked after a prior invocation of isReleasable or block returns true.

[leonard84]

I tried to test if this branch fixes the issue in Spock's CI jobs with the help of jitpack.io. Unfortunately, it fails to build.
As they only use Java 8 by default https://docs.jitpack.io/building/#java-version

[sormuras]

There was a time we used to provide a custom .jitpack.yml file...

• Remove Jitpack usage and config #2815

[sormuras]

I went ahead and (temporarily) added such a configuration file to your branch in order to fix the build, right? Or does it have to be present in the main branch? Let's find out.

.jitpack.yml could read:

jdk:
  - openjdk21
before_install:
  - sdk install java 21.0.2-open
  - sdk use java 21.0.2-open
install:
  - ./gradlew --version
  - ./gradlew publishToMavenLocal -x test

[sormuras]

https://jitpack.io/com/github/junit-team/junit5/leo~fix-potential-lock-isssue-SNAPSHOT/build.log now reports:

Git error. Retrying.
Git error. Max repo size 500MB exceeded

Progress... now the Gradle Daemon "stalls" or so:
https://jitpack.io/com/github/junit-team/junit5/leo~fix-potential-lock-isssue-r5.11.0-ge71983c-15/build.log

[...]
Build tool exit code: 0
Looking for artifacts...
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF-8 -Dhttps.protocols=TLSv1.2
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF-8 -Dhttps.protocols=TLSv1.2
Could not write standard input to Gradle build daemon.
java.io.IOException: Stream closed
	at java.lang.ProcessBuilder$NullOutputStream.write(ProcessBuilder.java:433)
	at java.io.OutputStream.write(OutputStream.java:116)
[...]

[leonard84]

The actual build was successful:

BUILD SUCCESSFUL in 2m 43s
441 actionable tasks: 299 executed, 140 from cache, 2 up-to-date
Configuration cache entry stored.
Build tool exit code: 0

I think this comes from jitpack

Looking for artifacts...

as well as the actual error a bit further down

Unrecognized option: --add-exports
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

From the help message it looks like they use an ancient gradle version to do something with the artifacts.

https://docs.gradle.org/4.8.1/userguide/gradle_daemon.html

Maybe it would help if we reset Java back to 8 after building.

[leonard84]

Back to

Git error. Retrying.
Git error. Max repo size 500MB exceeded

did you do something to fix it?

[sormuras]

Mh, nothing special. Let me touch the file to trigger a recompilation...

[sormuras]

Stop! java 8.0.382-open is not available.

https://jitpack.io/com/github/junit-team/junit5/leo~fix-potential-lock-isssue-r5.11.0-gc4a1e22-18/build.log

[leonard84]

Yeah, I don't have sdk man, so I was just using whatever version Gemini suggested for Java 8.

[marcphilipp]

Thanks for fixing that! I wonder why this is only surfacing now... 🤷

[sbrannen]

Reopening to backport to the 5.10.x branch:

Oops. This is a PR which cannot be reopened.

See #3988 instead.