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

BlockHound does not flag Thread.sleep(long, int) since JDK 21 #394

Closed
Badbond opened this issue Jan 30, 2024 · 2 comments · Fixed by #395
Closed

BlockHound does not flag Thread.sleep(long, int) since JDK 21 #394

Badbond opened this issue Jan 30, 2024 · 2 comments · Fixed by #395
Assignees

Comments

@Badbond
Copy link
Contributor

Badbond commented Jan 30, 2024

With the introduction of Virtual Threads in JDK 21, the implementation of the Thread class has changed. As such, invocations of Thread.sleep(long, int) are no longer flagged by BlockHound. Note that TimeUnit.sleep also invokes this method.

Expected Behavior

Invocations of Thread.sleep(long, int) are flagged.

Actual Behavior

Invocations of Thread.sleep(long, int) are not flagged.

Steps to Reproduce

Consider the following JUnit test. This test passes on JDK <= 20, but fails for JDK 21.

  @Test
  void threadSleepInvocation() {
    BlockHound.install();
    Mono.fromRunnable(
            () -> {
              try {
                Thread.sleep(1, 1);
              } catch (InterruptedException e) {
                fail(e);
              }
            })
        .subscribeOn(Schedulers.parallel())
        .as(StepVerifier::create)
        .verifyErrorSatisfies(
            throwable ->
                assertThat(throwable)
                    .isInstanceOf(BlockingOperationError.class)
                    .hasMessageContaining("java.lang.Thread.sleep"));
  }

Possible Solution

With JDK <= 20, Thread.sleep(long, int) would call Thread.sleep(long) and as such both usages would be flagged.

With JDK >= 21, both methods call Thread.sleep0(long) (iff not a VirtualThread). Similar to conditionally adding signatures for JDK >= 9, we can add the signature for sleep0(J)V for JDK >= 21. However, I am not yet familiar with the stance of Reactor on the severity of blocking calls in virtual threads, given the recent virtual threads support of BoundedElasticScheduler. Perhaps a different strategy should be taken into account for virtual threads more widely.

I am eager to contribute a PR if simply flagging Thread.sleep0(long, int) invocations is acceptable.

Your Environment

Dependencies:

  • BlockHound: 1.0.8.RELEASE
  • Reactor BOM: 2023.0.2
  • ByteBuddy Parent: 1.14.11

JVM version (java -version):

openjdk version "21.0.2" 2024-01-16 LTS
OpenJDK Runtime Environment Temurin-21.0.2+13 (build 21.0.2+13-LTS)
OpenJDK 64-Bit Server VM Temurin-21.0.2+13 (build 21.0.2+13-LTS, mixed mode, sharing)

OS and version (eg uname -a):

Linux PDSoels-Latitude-5421 5.15.0-91-generic #101~20.04.1-Ubuntu SMP Thu Nov 16 14:22:28 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
@pderop pderop self-assigned this Jan 30, 2024
@pderop
Copy link
Contributor

pderop commented Jan 30, 2024

@Badbond , thanks for reporting, I will take a look.

@pderop
Copy link
Contributor

pderop commented Jan 31, 2024

@Badbond ,

That would be great if you want to contribute! indeed, looking at Thread.java from jdk21, a kind of put("sleep0", singleton("(J)V")); should resolve the issue.

Regarding virtual threads, if I'm correct, boundedElastic() runs tasks on virtual threads , but BlockHound do not check blocking methods from boundedElastic() threads, so it should be ok.
In any case, if there are other issues related to loom that need to be addressed, we can do it in some other PRs, so feel free to go ahead with a PR that is fixing this current issue.

thanks !

pderop pushed a commit that referenced this issue Feb 6, 2024
Since JDK19, java.lang.Thread's methods have changed to support virtual threads. Specifically, for non-virtual threads, sleep(long) invokes sleep0(long) and yield() invokes yield0(). Moreover, since JDK21, sleep(long, int) and sleep(Duration) directly call sleep0(long) as opposed to sleep(long).

Now, to identify all blocking java.lang.Thread calls, BlockHound will flag invocations of sleep0(long) and yield0() as opposed to sleep(long) and yield() when running on JDK19 or greater.

Fixes #394.
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 a pull request may close this issue.

2 participants