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

Flag inner Thread methods for JDK 19 and greater #395

Merged
merged 3 commits into from
Feb 6, 2024
Merged

Flag inner Thread methods for JDK 19 and greater #395

merged 3 commits into from
Feb 6, 2024

Conversation

Badbond
Copy link
Contributor

@Badbond Badbond commented Jan 31, 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.

@Badbond
Copy link
Contributor Author

Badbond commented Jan 31, 2024

Verified against our codebase using ./gradlew --no-daemon -Pversion=1.0.9.BUILD-SNAPSHOT publishToMavenLocal. Note, this does change the method to instrument for JDK19 or greater, causing the error message to be different.

Also, as opposed to what was discussed in #394, the change now target JDK19+ as opposed to JDK21+. This is because the sleep0 and yield0 methods were already introduced back then. It felt more natural to already support it from that moment on, given the 0 suffixed methods are smaller in scope and will not be used by virtual threads.

Lastly, based on the contribution guidelines, I updated the license headers for changed files.

@pderop pderop self-requested a review January 31, 2024 16:18
@pderop pderop added this to the 1.0.9.RELEASE milestone Jan 31, 2024
@pderop pderop added the type/enhancement A general enhancement label Jan 31, 2024
@pderop
Copy link
Contributor

pderop commented Feb 5, 2024

@Badbond ,

Thanks for the PR and thanks for following our contribution guidelines diligently.
Truly appreciate your attention to details, however, we should maintain the current license headers due to internal reasons.

Could you then please revert the changes related to license headers and keep the copyright unchanged ?
After that, then let's merge this PR !

thanks.

@Badbond
Copy link
Contributor Author

Badbond commented Feb 6, 2024

Surely! Reverted the commit.

Copy link
Contributor

@pderop pderop left a comment

Choose a reason for hiding this comment

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

@Badbond , thanks for the PR !
LGTM

@pderop pderop merged commit 882a753 into reactor:master Feb 6, 2024
2 checks passed
@Badbond Badbond deleted the 394-flagThreadSleep0ForJDK21 branch February 6, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlockHound does not flag Thread.sleep(long, int) since JDK 21
2 participants