[SPARK-38446][Core] Fix deadlock between ExecutorClassLoader and FileDownloadCallback caused by Log4j by yaooqinn · Pull Request #35765 · apache/spark

yaooqinn · 2022-03-08T09:59:56Z

What changes were proposed in this pull request?

While log4j.ignoreTCL/log4j2.ignoreTCL is false, which is the default, it uses the context ClassLoader for the current Thread, see org.apache.logging.log4j.util.LoaderUtil.loadClass. While ExecutorClassLoader try to loadClass through remotely though the FileDownload, if error occurs, we will long on debug level, and log4j...LoaderUtil will be blocked by ExecutorClassLoader acquired classloading lock.

Fortunately, it only happens when ThresholdFilter's level is debug.

or we can set log4j.ignoreTCL/log4j2.ignoreTCL to true, but I don't know what else it will cause.

So in this PR, I simply remove the debug log which cause this deadlock

Why are the changes needed?

fix deadlock

Does this PR introduce any user-facing change?

no

How was this patch tested?

apache/kyuubi#2046 (comment), with a ut in kyuubi project, resolved(https://github.com/apache/incubator-kyuubi/actions/runs/1950222737)

Additional Resources

ut.jstack.txt

…adCallback caused by Log4j

yaooqinn · 2022-03-08T10:08:06Z

cc @cloud-fan @HyukjinKwon @dongjoon-hyun

dongjoon-hyun · 2022-03-08T11:28:46Z

cc @viirya

dongjoon-hyun · 2022-03-08T11:30:18Z

core/src/main/scala/org/apache/spark/rpc/netty/NettyRpcEnv.scala

    }

    override def onFailure(streamId: String, cause: Throwable): Unit = {
-      logDebug(s"Error downloading stream $streamId.", cause)


So, the deadlock happens when we use DEBUG log level?

the log4j will loadClass again with ExecutorClassLoader, if the loglevel does not satisfy, it is skipped

Before the deadlock occurs, I see something very similar with #10337 (comment)

cloud-fan · 2022-03-08T13:09:29Z

cc @Ngone51 @jiangxb1987

viirya · 2022-03-08T18:56:19Z

Hmm, after I read the description, I think shouldn't we fix it at ExecutorClassLoader? E.g., falling back to the default class loader as log4j2.ignoreTCL?

yaooqinn · 2022-03-09T02:24:54Z

thanks, @viirya,
Firstly we cannot fall back directly before ExecutorClassLoader tries remote classes via log4j2.ignoreTCL, it fails in the middle of fetching remote classes. Then we try to log a debug message where we shall skip via log4j2.ignoreTCL. As we cannot determine in ExecutorClassLoader whether the caller for loading classes is from Log4j. I don't know how we play the trick to ignore log4j2.ignoreTCL first then respect it later.

dongjoon-hyun · 2022-04-03T06:27:02Z

cc @MaxGekk since he is a release manager.

dongjoon-hyun

+1, LGTM. Since this is a removal of log, there is no functional regression .
Let me merge this and I hope this can unblock Apache Kyuubi project and Apache Spark 3.3.0 release.

Thank you, @yaooqinn , @srowen , @viirya , @cloud-fan .

…DownloadCallback caused by Log4j ### What changes were proposed in this pull request? While `log4j.ignoreTCL/log4j2.ignoreTCL` is false, which is the default, it uses the context ClassLoader for the current Thread, see `org.apache.logging.log4j.util.LoaderUtil.loadClass`. While ExecutorClassLoader try to loadClass through remotely though the FileDownload, if error occurs, we will long on debug level, and `log4j...LoaderUtil` will be blocked by ExecutorClassLoader acquired classloading lock. Fortunately, it only happens when ThresholdFilter's level is `debug`. or we can set `log4j.ignoreTCL/log4j2.ignoreTCL` to true, but I don't know what else it will cause. So in this PR, I simply remove the debug log which cause this deadlock ### Why are the changes needed? fix deadlock ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? apache/kyuubi#2046 (comment), with a ut in kyuubi project, resolved(https://github.com/apache/incubator-kyuubi/actions/runs/1950222737) ### Additional Resources [ut.jstack.txt](https://github.com/apache/spark/files/8206457/ut.jstack.txt) Closes #35765 from yaooqinn/SPARK-38446. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit aef6745) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

yaooqinn · 2022-04-03T17:34:51Z

thanks @dongjoon-hyun and all

…DownloadCallback caused by Log4j ### What changes were proposed in this pull request? While `log4j.ignoreTCL/log4j2.ignoreTCL` is false, which is the default, it uses the context ClassLoader for the current Thread, see `org.apache.logging.log4j.util.LoaderUtil.loadClass`. While ExecutorClassLoader try to loadClass through remotely though the FileDownload, if error occurs, we will long on debug level, and `log4j...LoaderUtil` will be blocked by ExecutorClassLoader acquired classloading lock. Fortunately, it only happens when ThresholdFilter's level is `debug`. or we can set `log4j.ignoreTCL/log4j2.ignoreTCL` to true, but I don't know what else it will cause. So in this PR, I simply remove the debug log which cause this deadlock ### Why are the changes needed? fix deadlock ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? apache/kyuubi#2046 (comment), with a ut in kyuubi project, resolved(https://github.com/apache/incubator-kyuubi/actions/runs/1950222737) ### Additional Resources [ut.jstack.txt](https://github.com/apache/spark/files/8206457/ut.jstack.txt) Closes apache#35765 from yaooqinn/SPARK-38446. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit aef6745) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

[SPARK-38446] Fix deadlock between ExecutorClassLoader and FileDownlo…

fbf1482

…adCallback caused by Log4j

github-actions bot added the CORE label Mar 8, 2022

yaooqinn mentioned this pull request Mar 8, 2022

[KYUUBI #2043] Upgrade log4j/2.x/ to 2.17.2 apache/kyuubi#2046

Closed

3 tasks

dongjoon-hyun reviewed Mar 8, 2022

View reviewed changes

srowen approved these changes Apr 3, 2022

View reviewed changes

dongjoon-hyun approved these changes Apr 3, 2022

View reviewed changes

dongjoon-hyun closed this in aef6745 Apr 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[SPARK-38446][Core] Fix deadlock between ExecutorClassLoader and FileDownloadCallback caused by Log4j#35765

[SPARK-38446][Core] Fix deadlock between ExecutorClassLoader and FileDownloadCallback caused by Log4j#35765
yaooqinn wants to merge 1 commit intoapache:masterfrom
yaooqinn:SPARK-38446

yaooqinn commented Mar 8, 2022 •

edited

Loading

Uh oh!

yaooqinn commented Mar 8, 2022

Uh oh!

dongjoon-hyun commented Mar 8, 2022

Uh oh!

dongjoon-hyun Mar 8, 2022

Uh oh!

yaooqinn Mar 8, 2022

Uh oh!

yaooqinn Mar 8, 2022

Uh oh!

cloud-fan commented Mar 8, 2022

Uh oh!

viirya commented Mar 8, 2022

Uh oh!

yaooqinn commented Mar 9, 2022

Uh oh!

dongjoon-hyun commented Apr 3, 2022 •

edited

Loading

Uh oh!

dongjoon-hyun left a comment

Uh oh!

yaooqinn commented Apr 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

Conversation

yaooqinn commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Additional Resources

Uh oh!

yaooqinn commented Mar 8, 2022

Uh oh!

dongjoon-hyun commented Mar 8, 2022

Uh oh!

dongjoon-hyun Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

yaooqinn Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

yaooqinn Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Mar 8, 2022

Uh oh!

viirya commented Mar 8, 2022

Uh oh!

yaooqinn commented Mar 9, 2022

Uh oh!

dongjoon-hyun commented Apr 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Apr 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yaooqinn commented Mar 8, 2022 •

edited

Loading

dongjoon-hyun commented Apr 3, 2022 •

edited

Loading