[SPARK-26665][Core]Fix a bug that BlockTransferService.fetchBlockSync may hang forever #23590

zsxwing · 2019-01-18T21:59:05Z

What changes were proposed in this pull request?

ByteBuffer.allocate may throw OutOfMemoryError when the block is large but no enough memory is available. However, when this happens, right now BlockTransferService.fetchBlockSync will just hang forever as its BlockFetchingListener. onBlockFetchSuccess doesn't complete Promise.

This PR catches Throwable and uses the error to complete Promise.

How was this patch tested?

Added a unit test. Since I cannot make ByteBuffer.allocate throw OutOfMemoryError, I passed a negative size to make ByteBuffer.allocate fail. Although the error type is different, it should trigger the same code path.

SparkQA · 2019-01-19T01:38:35Z

Test build #101413 has finished for PR 23590 at commit da1aaac.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-01-19T08:00:00Z

Retest this please.

SparkQA · 2019-01-19T11:55:50Z

Test build #101427 has finished for PR 23590 at commit da1aaac.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-01-19T19:20:26Z

Retest this please.

SparkQA · 2019-01-20T00:03:15Z

Test build #101437 has finished for PR 23590 at commit da1aaac.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mridulm · 2019-01-20T12:26:17Z

core/src/main/scala/org/apache/spark/network/BlockTransferService.scala

+                ret.flip()
+                result.success(new NioManagedBuffer(ret))
+              } catch {
+                case e: Throwable => result.failure(e)


An alternative to catching and ignoring Throwable (after result.failures) is to use a boolean flag to indicate if result.success was signalled - and if not, invoke result.failure in finally.

var success = false try { ... result.success(...) success = true } finally { if (! success) result.failure(e) }

we need to get the e, I think the catch is necessary here.

Good point - it is required for the failure arg.
But I dont like the throwable being ignored in the catch once failure is signalled.
For example, interruption, OOM, thread death, etc - we should rethrow it once promise is updated.

it's not ignored, the exception will be rethrown in ThreadUtils.awaitResult at the end of fetchBlockSync. Maybe we should not use Promise, and do it manually to be more explicit.

Looks like this is a common anti pattern in spark; the issue is basically thread/pool specific exceptions start leaking to other threads.
Though my basic concern was regarding things like OOM, and they do get propagated - so I guess we can live with this for now.

Promise is the most convenient way to turn an async listener to a Scala Future in order to call blocking APIs provided by Future.

Yeah, that's propagated and the code calling fetchBlockSync will handle it.

cloud-fan · 2019-01-21T09:21:15Z

core/src/test/scala/org/apache/spark/network/BlockTransferServiceSuite.scala

+          listener: BlockFetchingListener,
+          tempFileManager: DownloadFileManager): Unit = {
+        // Notify BlockFetchingListener with a bad ManagedBuffer asynchronously
+        new Thread() {


why do we create a thread here?

Otherwise, BlockFetchingListener will be called in the same thread and fail at once before Thread.awaitResult is called. Then the test won't be able to reproduce the issue.

cloud-fan · 2019-01-21T09:21:39Z

good catch! LGTM except a comment for the test.

zsxwing · 2019-01-22T16:59:31Z

Thanks! Merging to master and 2.4.

…c may hang forever ## What changes were proposed in this pull request? `ByteBuffer.allocate` may throw `OutOfMemoryError` when the block is large but no enough memory is available. However, when this happens, right now BlockTransferService.fetchBlockSync will just hang forever as its `BlockFetchingListener. onBlockFetchSuccess` doesn't complete `Promise`. This PR catches `Throwable` and uses the error to complete `Promise`. ## How was this patch tested? Added a unit test. Since I cannot make `ByteBuffer.allocate` throw `OutOfMemoryError`, I passed a negative size to make `ByteBuffer.allocate` fail. Although the error type is different, it should trigger the same code path. Closes #23590 from zsxwing/SPARK-26665. Authored-by: Shixiong Zhu <[email protected]> Signed-off-by: Shixiong Zhu <[email protected]> (cherry picked from commit 66450bb) Signed-off-by: Shixiong Zhu <[email protected]>

dongjoon-hyun · 2019-01-22T17:14:44Z

Hi, @zsxwing , @cloud-fan , @mridulm .
Since the underlying code seems to be added by SPARK-22062 at Spark 2.3.0, I'm wondering if we can have this at branch-2.3.
cc @maropu since he is a release manager for 2.3.3.

zsxwing · 2019-01-22T17:17:20Z

@dongjoon-hyun I think SPARK-22062 just moved codes. This is a long standing issue.

dongjoon-hyun · 2019-01-22T17:24:10Z

Oh, thank you for the info. Then, can we have this to branch-2.3?

…c may hang forever ## What changes were proposed in this pull request? `ByteBuffer.allocate` may throw `OutOfMemoryError` when the block is large but no enough memory is available. However, when this happens, right now BlockTransferService.fetchBlockSync will just hang forever as its `BlockFetchingListener. onBlockFetchSuccess` doesn't complete `Promise`. This PR catches `Throwable` and uses the error to complete `Promise`. ## How was this patch tested? Added a unit test. Since I cannot make `ByteBuffer.allocate` throw `OutOfMemoryError`, I passed a negative size to make `ByteBuffer.allocate` fail. Although the error type is different, it should trigger the same code path. Closes #23590 from zsxwing/SPARK-26665. Authored-by: Shixiong Zhu <[email protected]> Signed-off-by: Shixiong Zhu <[email protected]>

zsxwing · 2019-01-22T17:35:36Z

@dongjoon-hyun Sure. This is a pretty safe change. I just pushed to branch-2.3.

dongjoon-hyun · 2019-01-22T17:57:14Z

Great! Thank you so much, @zsxwing .

…c may hang forever ## What changes were proposed in this pull request? `ByteBuffer.allocate` may throw `OutOfMemoryError` when the block is large but no enough memory is available. However, when this happens, right now BlockTransferService.fetchBlockSync will just hang forever as its `BlockFetchingListener. onBlockFetchSuccess` doesn't complete `Promise`. This PR catches `Throwable` and uses the error to complete `Promise`. ## How was this patch tested? Added a unit test. Since I cannot make `ByteBuffer.allocate` throw `OutOfMemoryError`, I passed a negative size to make `ByteBuffer.allocate` fail. Although the error type is different, it should trigger the same code path. Closes apache#23590 from zsxwing/SPARK-26665. Authored-by: Shixiong Zhu <[email protected]> Signed-off-by: Shixiong Zhu <[email protected]>

…checked exception missed This is very like #23590 . `ByteBuffer.allocate` may throw `OutOfMemoryError` when the response is large but no enough memory is available. However, when this happens, `TransportClient.sendRpcSync` will just hang forever if the timeout set to unlimited. This PR catches `Throwable` and uses the error to complete `SettableFuture`. I tested in my IDE by setting the value of size to -1 to verify the result. Without this patch, it won't be finished until timeout (May hang forever if timeout set to MAX_INT), or the expected `IllegalArgumentException` will be caught. ```java Override public void onSuccess(ByteBuffer response) { try { int size = response.remaining(); ByteBuffer copy = ByteBuffer.allocate(size); // set size to -1 in runtime when debug copy.put(response); // flip "copy" to make it readable copy.flip(); result.set(copy); } catch (Throwable t) { result.setException(t); } } ``` Closes #24964 from LantaoJin/SPARK-28160. Lead-authored-by: LantaoJin <[email protected]> Co-authored-by: lajin <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 0e42100) Signed-off-by: Sean Owen <[email protected]>

…checked exception missed ## What changes were proposed in this pull request? This is very like apache#23590 . `ByteBuffer.allocate` may throw `OutOfMemoryError` when the response is large but no enough memory is available. However, when this happens, `TransportClient.sendRpcSync` will just hang forever if the timeout set to unlimited. This PR catches `Throwable` and uses the error to complete `SettableFuture`. ## How was this patch tested? I tested in my IDE by setting the value of size to -1 to verify the result. Without this patch, it won't be finished until timeout (May hang forever if timeout set to MAX_INT), or the expected `IllegalArgumentException` will be caught. ```java Override public void onSuccess(ByteBuffer response) { try { int size = response.remaining(); ByteBuffer copy = ByteBuffer.allocate(size); // set size to -1 in runtime when debug copy.put(response); // flip "copy" to make it readable copy.flip(); result.set(copy); } catch (Throwable t) { result.setException(t); } } ``` Closes apache#24964 from LantaoJin/SPARK-28160. Lead-authored-by: LantaoJin <[email protected]> Co-authored-by: lajin <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…c may hang forever ## What changes were proposed in this pull request? `ByteBuffer.allocate` may throw `OutOfMemoryError` when the block is large but no enough memory is available. However, when this happens, right now BlockTransferService.fetchBlockSync will just hang forever as its `BlockFetchingListener. onBlockFetchSuccess` doesn't complete `Promise`. This PR catches `Throwable` and uses the error to complete `Promise`. ## How was this patch tested? Added a unit test. Since I cannot make `ByteBuffer.allocate` throw `OutOfMemoryError`, I passed a negative size to make `ByteBuffer.allocate` fail. Although the error type is different, it should trigger the same code path. Closes apache#23590 from zsxwing/SPARK-26665. Authored-by: Shixiong Zhu <[email protected]> Signed-off-by: Shixiong Zhu <[email protected]> (cherry picked from commit 66450bb) Signed-off-by: Shixiong Zhu <[email protected]>

…checked exception missed This is very like apache#23590 . `ByteBuffer.allocate` may throw `OutOfMemoryError` when the response is large but no enough memory is available. However, when this happens, `TransportClient.sendRpcSync` will just hang forever if the timeout set to unlimited. This PR catches `Throwable` and uses the error to complete `SettableFuture`. I tested in my IDE by setting the value of size to -1 to verify the result. Without this patch, it won't be finished until timeout (May hang forever if timeout set to MAX_INT), or the expected `IllegalArgumentException` will be caught. ```java Override public void onSuccess(ByteBuffer response) { try { int size = response.remaining(); ByteBuffer copy = ByteBuffer.allocate(size); // set size to -1 in runtime when debug copy.put(response); // flip "copy" to make it readable copy.flip(); result.set(copy); } catch (Throwable t) { result.setException(t); } } ``` Closes apache#24964 from LantaoJin/SPARK-28160. Lead-authored-by: LantaoJin <[email protected]> Co-authored-by: lajin <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 0e42100) Signed-off-by: Sean Owen <[email protected]>

Fix a bug that BlockTransferService.fetchBlockSync may hang forever

da1aaac

zsxwing changed the title ~~Fix a bug that BlockTransferService.fetchBlockSync may hang forever~~ [SPARK-26665][Core]Fix a bug that BlockTransferService.fetchBlockSync may hang forever Jan 18, 2019

zsxwing requested a review from cloud-fan January 18, 2019 22:44

mridulm reviewed Jan 20, 2019

View reviewed changes

cloud-fan reviewed Jan 21, 2019

View reviewed changes

cloud-fan approved these changes Jan 22, 2019

View reviewed changes

asfgit closed this in 66450bb Jan 22, 2019

zsxwing deleted the SPARK-26665 branch January 22, 2019 17:10

LantaoJin mentioned this pull request Jun 25, 2019

[SPARK-28160][CORE] Fix a bug that callback function may hang when unchecked exception missed #24964

Closed

AngersZhuuuu mentioned this pull request Oct 25, 2019

[SPARK-29551][CORE] Fix a bug about fetch failed when an executor is lost #26206

Closed

[SPARK-26665][Core]Fix a bug that BlockTransferService.fetchBlockSync may hang forever #23590

[SPARK-26665][Core]Fix a bug that BlockTransferService.fetchBlockSync may hang forever #23590

Uh oh!

Conversation

zsxwing commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jan 19, 2019

Uh oh!

dongjoon-hyun commented Jan 19, 2019

Uh oh!

SparkQA commented Jan 19, 2019

Uh oh!

dongjoon-hyun commented Jan 19, 2019

Uh oh!

SparkQA commented Jan 20, 2019

Uh oh!

mridulm Jan 20, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

mridulm Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mridulm Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

zsxwing Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

zsxwing Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

zsxwing Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 21, 2019

Uh oh!

zsxwing commented Jan 22, 2019

Uh oh!

dongjoon-hyun commented Jan 22, 2019

Uh oh!

zsxwing commented Jan 22, 2019

Uh oh!

dongjoon-hyun commented Jan 22, 2019

Uh oh!

zsxwing commented Jan 22, 2019

Uh oh!

dongjoon-hyun commented Jan 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zsxwing commented Jan 18, 2019 •

edited

Loading

mridulm Jan 21, 2019 •

edited

Loading

cloud-fan Jan 21, 2019 •

edited

Loading