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.

Sign up for GitHub

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

Jump to bottom

Remove usages of Unsafe #215

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Remove usages of Unsafe #215

wants to merge 1 commit into from

Conversation

dmlloyd
Copy link
Contributor

@dmlloyd dmlloyd commented Jan 7, 2025 •
edited
Loading

Remove all usages of Unsafe in favor of supported APIs.

To use the thread-local resetter will now require --add-opens java.base/java.lang=org.jboss.threads (or ALL-UNNAMED if not using JDK modules).

Java 25+ reports this:

WARNING: A terminally deprecated method in sun.misc.Unsafe has been called
WARNING: sun.misc.Unsafe::objectFieldOffset has been called by org.jboss.threads.JBossExecutors (jar:file:/Volumes/Case-sensitive/david/.../org/jboss/threads/jboss-threads-3.8.0.Final.jar!/org/jboss/threads/ContextHandler.class)
WARNING: Please consider reporting this to the maintainers of class org.jboss.threads.JBossExecutors
WARNING: sun.misc.Unsafe::objectFieldOffset will be removed in a future release

@dmlloyd dmlloyd requested a review from franz1981 January 7, 2025 14:54
franz1981
franz1981 reviewed Jan 7, 2025
View reviewed changes
src/main/java/org/jboss/threads/ThreadLocalResettingRunnable.java Outdated
} catch (NoSuchFieldException e) {
throw new NoSuchFieldError(e.getMessage());
}
}

static void run() {
final Thread thread = Thread.currentThread();
JBossExecutors.unsafe.putObject(thread, threadLocalMapOffs, null);
JBossExecutors.unsafe.putObject(thread, inheritableThreadLocalMapOffs, null);
try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We gotta ask @galderz how this perform in native

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect it to be OK: AFAIK, VarHandle is using these method handles internally, and we're acquiring them in static init. My expectation is that the calls will be inlined.

However, either way it's not going to work without --add-opens java.base/java.lang=org.jboss.threads or ...=ALL-UNNAMED. That's the real sticky part.

@dmlloyd dmlloyd mentioned this pull request Jan 8, 2025
Open
@dmlloyd dmlloyd force-pushed the rip-unsafe branch 2 times, most recently from 3ccbcaa to 6b81015 Compare January 13, 2025 15:28
@dmlloyd
Copy link
Contributor Author

dmlloyd commented Jan 13, 2025 •
edited
Loading

The main outstanding problem here is that we need a jboss-parent-pom release (48) which includes support for Java 24. See jboss/jboss-parent-pom#391 for details.

@geoand geoand mentioned this pull request Jan 15, 2025
Open
@dmlloyd
Copy link
Contributor Author

dmlloyd commented Jan 16, 2025

Updated so that we continue to use Unsafe for accessing private JDK things until Java 24, at which time we switch to VarHandle, requiring an explicit --add-opens.

@dmlloyd
Copy link
Contributor Author

dmlloyd commented Jan 23, 2025

This now just needs:

  • Java 24 release to be finalized
  • GitHub runners to support Java 24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@franz1981 franz1981 franz1981 left review comments

@jamezp jamezp jamezp left review comments

@Sanne Sanne Sanne left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmlloyd @Sanne @jamezp @franz1981