[SPARK-48218][CORE] TransportClientFactory.createClient may NPE cause FetchFailedException #46506

cxzl25 · 2024-05-09T10:54:41Z

What changes were proposed in this pull request?

This PR aims to add a check for TransportChannelHandler to be non-null in the TransportClientFactory.createClient method.

Why are the changes needed?

Line 178 synchronized (handler) , handler == null

org.apache.spark.network.client.TransportClientFactory#createClient(java.lang.String, int, boolean)

      TransportChannelHandler handler = cachedClient.getChannel().pipeline()
        .get(TransportChannelHandler.class);
      synchronized (handler) {
        handler.getResponseHandler().updateTimeOfLastRequest();
      }

org.apache.spark.shuffle.FetchFailedException
	at org.apache.spark.storage.ShuffleBlockFetcherIterator.throwFetchFailedException(ShuffleBlockFetcherIterator.scala:1180)
	at org.apache.spark.storage.ShuffleBlockFetcherIterator.next(ShuffleBlockFetcherIterator.scala:913)
	at org.apache.spark.storage.ShuffleBlockFetcherIterator.next(ShuffleBlockFetcherIterator.scala:84)
	at org.apache.spark.util.CompletionIterator.next(CompletionIterator.scala:29)

Caused by: java.lang.NullPointerException
	at org.apache.spark.network.client.TransportClientFactory.createClient(TransportClientFactory.java:178)
	at org.apache.spark.network.shuffle.ExternalBlockStoreClient.lambda$fetchBlocks$0(ExternalBlockStoreClient.java:128)
	at org.apache.spark.network.shuffle.RetryingBlockTransferor.transferAllOutstanding(RetryingBlockTransferor.java:154)
	at org.apache.spark.network.shuffle.RetryingBlockTransferor.start(RetryingBlockTransferor.java:133)
	at org.apache.spark.network.shuffle.ExternalBlockStoreClient.fetchBlocks(ExternalBlockStoreClient.java:139)

Does this PR introduce any user-facing change?

No

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

No

dongjoon-hyun · 2024-05-09T15:41:27Z

common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java

@@ -169,8 +169,10 @@ public TransportClient createClient(String remoteHost, int remotePort, boolean f
      // this code was able to update things.
      TransportChannelHandler handler = cachedClient.getChannel().pipeline()
        .get(TransportChannelHandler.class);
-      synchronized (handler) {


Your error message doesn't match with code. Maybe, it came from your own fork?

Caused by: java.lang.NullPointerException at org.apache.spark.network.client.TransportClientFactory.createClient(TransportClientFactory.java:178)

dongjoon-hyun · 2024-05-09T15:42:45Z

common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java

@@ -169,8 +169,10 @@ public TransportClient createClient(String remoteHost, int remotePort, boolean f
      // this code was able to update things.
      TransportChannelHandler handler = cachedClient.getChannel().pipeline()
        .get(TransportChannelHandler.class);
-      synchronized (handler) {
-        handler.getResponseHandler().updateTimeOfLastRequest();
+      if (handler != null) {


I'm not sure if this is a safe replacement.

Although I don't know your code, if we need to check nullability, shall we do inside synchronized block?

Please let me know if you are observing synchronized(handler) line is the root cause of NPE. I assumed that NPE happens at handler.getRes....

synchronized will throw an NPE if called on null - and handler is a local variable at this point, so the null check should be fine.

This should be a fairly rare occurrence - if I am not wrong, this is due to socket close between the isActive call and the subsequent pipeline().get(TransportChannelHandler.class) call - but agree with @dongjoon-hyun, we have to make sure the exception is at the sychronized statement itself : which version was this observed against @cxzl25 ?

Thank you for the info.
Yes, it would be a correct fix if we have a matched line error message.

Thanks everyone for the quick comments!

I observed a small number of cases in the production environment.
This is a modified version based on Spark3.2, so the number of lines is a bit inconsistent.
However, when I checked the number of lines of code and decompiled the jar, they all pointed to synchronized (handler ) this line of code.

The NPE issue here is somewhat similar to SPARK-11865.
When another thread is closing the channel, isActive may return true, and may be false when checked for the second time. There is an intermediate state, resulting in NPE.

I tried to reproduce this issue in the latest version to match the number of rows, however it's a bit hard to do.

if (cachedClient != null && cachedClient.isActive() // true) { TransportChannelHandler handler = cachedClient.getChannel().pipeline() .get(TransportChannelHandler.class); synchronized (handler) { handler.getResponseHandler().updateTimeOfLastRequest(); } if (cachedClient.isActive() // false) {

Thanks for clarifying !

mridulm

+CC @yaooqinn as well who last reviewed some of this code.

mridulm · 2024-05-09T19:33:46Z

common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java

@@ -169,8 +169,10 @@ public TransportClient createClient(String remoteHost, int remotePort, boolean f
      // this code was able to update things.
      TransportChannelHandler handler = cachedClient.getChannel().pipeline()
        .get(TransportChannelHandler.class);
-      synchronized (handler) {
-        handler.getResponseHandler().updateTimeOfLastRequest();
+      if (handler != null) {


synchronized will throw an NPE if called on null - and handler is a local variable at this point, so the null check should be fine.

This should be a fairly rare occurrence - if I am not wrong, this is due to socket close between the isActive call and the subsequent pipeline().get(TransportChannelHandler.class) call - but agree with @dongjoon-hyun, we have to make sure the exception is at the sychronized statement itself : which version was this observed against @cxzl25 ?

mridulm

I am fine with the change, assuming the exception was observed at the right location - agree with @dongjoon-hyun, this is unclear right now.

See followup

mridulm

+CC @dongjoon-hyun

dongjoon-hyun

Sorry for being late. I was traveling.

dongjoon-hyun · 2024-05-15T16:34:21Z

Merged to master for Apache Spark 4.0.0-preview2-rc2.

Thank you, @cxzl25 , @mridulm , @yaooqinn .

…s null when creating client ### What changes were proposed in this pull request? `TransportClientFactory` checks whether `handler` is null when creating client. ### Why are the changes needed? There is a case that `cachedClient.isActive()` may return true and may return false when checked for the second time when another thread is closing the channel, which causes that the `handler` may be null. Therefore, `TransportClientFactory` should check whether handler is null when creating client. Backport apache/spark#46506. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? GA. Closes #2517 from SteNicholas/CELEBORN-1430. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

npe

0c78b1c

github-actions bot added the CORE label May 9, 2024

dongjoon-hyun reviewed May 9, 2024

View reviewed changes

mridulm previously approved these changes May 9, 2024

View reviewed changes

mridulm reviewed May 9, 2024

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-48218][SHUFFLE] TransportClientFactory.createClient may NPE cause FetchFailedException~~ [SPARK-48218][CORE] TransportClientFactory.createClient may NPE cause FetchFailedException May 9, 2024

yaooqinn approved these changes May 10, 2024

View reviewed changes

mridulm approved these changes May 13, 2024

View reviewed changes

dongjoon-hyun approved these changes May 15, 2024

View reviewed changes

dongjoon-hyun closed this in 6767053 May 15, 2024

SteNicholas mentioned this pull request May 16, 2024

[CELEBORN-1430] TransportClientFactory should check whether handler is null when creating client apache/celeborn#2517

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48218][CORE] TransportClientFactory.createClient may NPE cause FetchFailedException #46506

[SPARK-48218][CORE] TransportClientFactory.createClient may NPE cause FetchFailedException #46506

cxzl25 commented May 9, 2024

dongjoon-hyun May 9, 2024

dongjoon-hyun May 9, 2024

dongjoon-hyun May 9, 2024

mridulm May 9, 2024 •

edited

dongjoon-hyun May 9, 2024

cxzl25 May 10, 2024

mridulm May 13, 2024

mridulm left a comment

mridulm May 9, 2024 •

edited

mridulm left a comment

mridulm left a comment

dongjoon-hyun left a comment

dongjoon-hyun commented May 15, 2024

[SPARK-48218][CORE] TransportClientFactory.createClient may NPE cause FetchFailedException #46506

[SPARK-48218][CORE] TransportClientFactory.createClient may NPE cause FetchFailedException #46506

Conversation

cxzl25 commented May 9, 2024

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?

Was this patch authored or co-authored using generative AI tooling?

dongjoon-hyun May 9, 2024

Choose a reason for hiding this comment

dongjoon-hyun May 9, 2024

Choose a reason for hiding this comment

dongjoon-hyun May 9, 2024

Choose a reason for hiding this comment

mridulm May 9, 2024 • edited

Choose a reason for hiding this comment

dongjoon-hyun May 9, 2024

Choose a reason for hiding this comment

cxzl25 May 10, 2024

Choose a reason for hiding this comment

mridulm May 13, 2024

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

mridulm May 9, 2024 • edited

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented May 15, 2024

mridulm May 9, 2024 •

edited

mridulm May 9, 2024 •

edited