HADOOP-19574. Restore Subject propagation semantics for Java 22+ #7892
Conversation
github-actions
bot
added
YARN
MapReduce
HDFS
DistCp
trunk
Common
RBF
TOOLS
AWS
ABFS
DYNAMOMETER
labels
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
This is final piece for Java 24 / 25 support. Please take a look @pan3793 @slfan1989 @jojochuang @cnauroth @steveloughran |
I have a few experienced members to recommend for the review. @Hexiaoqiao @ayushtkn @szetszwo I need your help to collaboratively review this PR. Thank you very much! |
There was a problem hiding this comment.
Hi @stoty . It looks like Yetus is saying this won't apply to trunk. Does it need a rebase? I also just committed MAPREDUCE-7502, so if you rebase, the test fix won't need to be applied manually anymore.
|
Unfortunately Yetus cannot handle patches this big, I don't think we can get CI run via GitHub. |
|
The change LGTM. @stoty thank you for such great works! The current diff is huge, my only suggestion is to extract the |
| * Hadoop security heavily relies on the original behavior, as Subject is at the | ||
| * core of JAAS. This class wraps thread. It overrides start() and saves the | ||
| * Subject of the current thread, and wraps the payload in a | ||
| * Subject.doAs()/callAs() call to restorere it in the newly created Thread. |
| } | ||
|
|
||
| /** | ||
| * Behaves similar to {@link Thread#Thread(ThreadGroup, Runnable)} constructor. |
There was a problem hiding this comment.
nit: I think 'Behaves similarly' might be more correct. https://english.stackexchange.com/questions/189825/behaves-similar-to-or-behaves-similarly-to
There was a problem hiding this comment.
|
💔 -1 overall
This message was automatically generated. |
|
Conflict fixed on #7919 |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
@stoty , thanks for the update!
The current HadoopThread (now renamed to SubjectInheritingThread) intentionally follows the Thread syntax as close as possible for minimum changes in the callers.
In this patch, we are changing
new Thread(..)->new SubjectInheritingThread(..)
My suggestion is to change
new Thread(..)->new SubjectInheritingThread.newBuilder()...build()
If we do it in separated JIRAs, we will need to rewrite the almost all code this PR.
| */ | ||
| @Override | ||
| public final void run() { | ||
| SubjectUtil.doAs(startSubject, new PrivilegedAction<Void>() { |
There was a problem hiding this comment.
There is a performance problem in this PR since it makes all the threads calling doAs. For pre-Java 22, it will hurt the performance.
Unfortunately, that would only handle less than half of the thread creations. You cannot override the run() / work() method if you use a factory. This patch is huge enough as is. |
@stoty , builder is more flexible than constructors. We definitely can override method as today. //TestByteArrayManager
static class AllocatorThread extends Thread {// existing code
final AllocatorThread t = new AllocatorThread(arrayLength, bam);Instead of call // new code
final Thread t = SubjectInheritingThread.newBuilder()
.setThread(new AllocatorThread(arrayLength, bam))
.build(); |
|
I spent some time trying to implement that @szetszwo . At the core, we'd need a wrapper like this: ` }` run() and start() are wrappable, but Thread is full of final methods which cannot be overriden, and would not work on the wrapper (which is never actually started, only the wrapped thread is). Say, we want to call setDaemon() on the wrapper. Since we cannot override it, it would set the daemon flag on the wrapper, not the wrapped thread, which is never even start() ed , and wouldn't work. So a wrapper like this would invisibly break the Thread contract in many ways. |
|
@stoty , thanks for trying it out! The code should look like below: public class SubjectDoAsThread extends Thread {
private final Runnable runnable;
private Subject startSubject;
public static class Builder {
private ThreadGroup group;
private String name;
private Runnable runnable;
public Builder setThread(Thread thread) {
this.group = thread.getThreadGroup();
this.name = thread.getName();
this.runnable = thread;
return this;
}
public SubjectDoAsThread build() {
return new SubjectDoAsThread(group, name, runnable);
}
}
private SubjectDoAsThread(ThreadGroup group, String name, Runnable runnable) {
super(group, name);
this.runnable = Objects.requireNonNull(runnable, "runnable == null");
}
@Override
public final void start() {
startSubject = SubjectUtil.current();
super.start();
}
@Override
public final void run() {
SubjectUtil.doAs(startSubject, (PrivilegedAction<Void>) () -> {
runnable.run();
return null;
});
}
} |
|
That looks like it could work @szetszwo . There is still the issue that if there is any logic in the wrapped Thread's start() method, then it won't be run, |
|
I've spent some more tim on this @szetszwo , and I am even more convinced that using the Builder approach is NOT a good universal solution. I've implemented something close to your example above, then I randomy chose org.apache.hadoop.hdfs.server.namenode.ha.EditLogTailer to test it. Two problems became immediately apparent with the builder approach:
That means that only the Threads which set a Runner can be converted easily, the ones which subclass Thread would need to be evaulated on a case-by-case basis, which is exactly what I don't want to do due to both the time requirement and the error-prone nature of the process. @pan3793 @slfan1989 @jojochuang @cnauroth @steveloughran @Hexiaoqiao @ayushtkn @szetszwo Please review the current state of #7919 which is the same patch as this split into three and with some minor improvements. My plan is to rebase/reorder the #7919 . Please check #7919
|
|
@stoty , the code will look like below: diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/EditLogTailer.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/EditLogTailer.java
index cf416307f47..3260b770ac0 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/EditLogTailer.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/EditLogTailer.java
@@ -85,8 +85,8 @@ public class EditLogTailer {
public static final long DFS_HA_TAILEDITS_MAX_TXNS_PER_LOCK_DEFAULT =
Long.MAX_VALUE;
- private final EditLogTailerThread tailerThread;
-
+ private final SubjectDoAsThread<EditLogTailerThread> tailerThread;
+
private final Configuration conf;
private final FSNamesystem namesystem;
private final Iterator<RemoteNameNodeInfo> nnLookup;
@@ -180,7 +180,11 @@ public class EditLogTailer {
private Timer timer;
public EditLogTailer(FSNamesystem namesystem, Configuration conf) {
- this.tailerThread = new EditLogTailerThread();
+ final EditLogTailerThread t = new EditLogTailerThread();
+ this.tailerThread = SubjectDoAsThread.<EditLogTailerThread>newBuilder()
+ .setThread(new EditLogTailerThread())
+ .setOriginal(t)
+ .build();
this.conf = conf;
this.namesystem = namesystem;
this.timer = new Timer();
@@ -271,7 +275,7 @@ public void start() {
}
public void stop() throws IOException {
- tailerThread.setShouldRun(false);
+ tailerThread.getOriginal().setShouldRun(false);
tailerThread.interrupt();
try {
tailerThread.join();
@@ -664,6 +668,6 @@ public RemoteNameNodeInfo getCurrentNN() {
@VisibleForTesting
public void setShouldRunForTest(boolean shouldRun) {
- this.tailerThread.setShouldRun(shouldRun);
+ this.tailerThread.getOriginal().setShouldRun(shouldRun);
}
}
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/SubjectDoAsThread.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/SubjectDoAsThread.java
new file mode 100644
index 00000000000..edac81048a6
--- /dev/null
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/SubjectDoAsThread.java
@@ -0,0 +1,64 @@
+package org.apache.hadoop.hdfs.server.namenode.ha;
+
+import org.apache.hadoop.security.authentication.util.SubjectUtil;
+
+import javax.security.auth.Subject;
+import java.security.PrivilegedAction;
+import java.util.Objects;
+
+public class SubjectDoAsThread<T> extends Thread {
+ private final T original;
+ private final Runnable runnable;
+ private Subject startSubject;
+
+ public static class Builder<T> {
+ private ThreadGroup group;
+ private String name;
+ private Runnable runnable;
+ private T original;
+
+ public Builder<T> setThread(Thread thread) {
+ this.group = thread.getThreadGroup();
+ this.name = thread.getName();
+ this.runnable = thread;
+ return this;
+ }
+
+ public Builder<T> setOriginal(T original) {
+ this.original = original;
+ return this;
+ }
+
+ public SubjectDoAsThread<T> build() {
+ return new SubjectDoAsThread<>(group, name, original, runnable);
+ }
+ }
+
+ private SubjectDoAsThread(ThreadGroup group, String name, T original, Runnable runnable) {
+ super(group, name);
+ this.original = original;
+ this.runnable = Objects.requireNonNull(runnable, "runnable == null");
+ }
+
+ static <T> Builder<T> newBuilder() {
+ return new Builder<>();
+ }
+
+ @Override
+ public final void start() {
+ startSubject = SubjectUtil.current();
+ super.start();
+ }
+
+ @Override
+ public final void run() {
+ SubjectUtil.doAs(startSubject, (PrivilegedAction<Void>) () -> {
+ runnable.run();
+ return null;
+ });
+ }
+
+ public T getOriginal() {
+ return original;
+ }
+} |
|
@stoty Thank you for your contribution! However, I may not be able to thoroughly review this PR until next Monday or Tuesday. Currently, the JUnit 5 upgrade for the HDFS module has been mostly completed. I'm now working on completely removing the dependency on JUnit 4 from the trunk branch of the Hadoop project. This may take some time, as there are still a few missing pieces that need to be fixed. |
I think introducing Builder here is a kind of overdesign. @szetszwo do you have cases in your mind that can not be covered by @stoty's approach? If not, I strongly support @stoty's approach.
Thanks for splitting! I think HADOOP-19668 and HADOOP-19669 can be committed together; the current state is also fine.
I think it's already in good shape. |
|
Yes, the solution you propose works @szetszwo , but it's more complex and error-prone ot implement than the current one. Now we are juggling two thread objects, and it is very easy to start the wrong one, especially when you're converting hundreds of threads. |
|
@stoty I’d like to confirm with you whether you’d prefer us to create a separate development branch for you. Pan and I had an offline discussion on this topic. He believes the changes are manageable and therefore a separate branch may not be necessary. I think his point makes sense to some extent, but from the perspective of development efficiency and risk control, I would still recommend creating a dedicated development branch. This would allow us to iterate more quickly without impacting existing functionality. |
I don't think a separate branch is necessary, @slfan1989 . These changes are designed to be transparent, especially now that we've added a shortcut to avoid the subject manipulation for Java 21 and earlier, as suggested by @szetszwo . IF this breaks anything, we can find and fix that faster once it's in trunk. Any changes affecting Java 8/11 will be causght by the existing CI processes, and we can also start work on setting up CI for Java 17/21/25 and start working out any such issues. Users will alse have an easier time testing Hadoop with Java 21/25 in their use cases once it's supported on trunk. |
|
A related question @slfan1989 : |
@stoty Thank you for your reply, and also for your efforts in driving the Java 22+ upgrade. I hope the work goes smoothly, and I'll do my best to support it.
From my understanding, the trunk branch will soon drop support for Java 8 and Java 11, and move forward with Java 17 as the new baseline for development. We also plan to finalize the release scope of Hadoop 3.5 as soon as possible and initiate the corresponding release process. As for CI support for Java 21 and above, further discussion may be needed. I plan to respond to the previous DISCUSS thread early next week and continue driving the related efforts. In the meantime, input and suggestions from other community members would be very welcome. |
|
Kindly ping @szetszwo @steveloughran @cnauroth We need a final decision here. I think these two approaches are just different programming patterns, with no essential functionality difference. Java 25 is coming (scheduled on Sept 26, 2025), and I am eager for a Java 25-compatible Hadoop client to unblock downstream projects to support Java 25. |
|
@stoty , @pan3793 , Honestly, I do prefer adding a builder instead of having multiple constructors with different parameter combinations. Also, a builder can build subclasses but constructors can't. In addition, extending the However, I won't -1 if we are not adding builder. It is just a suggestion for your consideration. I am fine if other people have agreed to do it differently. |
… pre JDK22 Subject behaviour in Threads
…re JDK22 Subject behaviour in Threads
|
💔 -1 overall
This message was automatically generated. |
|
I have overwritten this PR with the split one, and rebased it to current trunk. |
Description of PR
HADOOP-19574. Restore Subject propagation semantics for Java 22+.
JDK 22 breaks subject propagation into new Threads.
This patch adds a new HadoopThread class which restores the pre JDK22 semantics, and replaces
most Thread objects in the code with HadoopThread.
The Daemon class is silimarly changed.
This is the -hopefully- final patch for full Java 24/25 support.
I went with the approach of replacing almost all Thread objects with HadoopThread, as most of the tests do not set any specific Principal, and it would be had to determine for every Thread usage whether Subject propagation is required for that instance.
While the patch is huge, 99% of it is just mechanically replacing Thread with HadoopThread, and the run() method with the work() method.
There are a few tests which check specific stack traces, which also needed updating.
I'm open to renaming anything if someone can come up with better names.
How was this patch tested?
There are a few failing tests, but those are either flakey or also fail on trunk and / or with Java 8 / 17.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?