[SPARK-28709][DSTREAMS] Fix StreamingContext leak through Streaming #25439

choojoyq · 2019-08-13T21:18:48Z

What changes were proposed in this pull request?

In my application spark streaming is restarted programmatically by stopping StreamingContext without stopping of SparkContext and creating/starting a new one. I use it for automatic detection of Kafka topic/partition changes and automatic failover in case of non fatal exceptions.

However i notice that after multiple restarts driver fails with OOM. During investigation of heap dump i figured out that StreamingContext object isn't cleared by GC after stopping.

There are several places which holds reference to it :

StreamingTab registers StreamingJobProgressListener which holds reference to Streaming Context directly to LiveListenerBus shared queue via ssc.sc.addSparkListener(listener) method invocation. However this listener isn't unregistered at stop method.
json handlers (/streaming/json and /streaming/batch/json) aren't unregistered in SparkUI, while they hold reference to StreamingJobProgressListener. Basically the same issue affects all the pages, i assume that renderJsonHandler should be added to pageToHandlers cache on attachPage method invocation in order to unregistered it as well on detachPage.
SparkUi holds reference to StreamingJobProgressListener in the corresponding local variable which isn't cleared after stopping of StreamingContext.

How was this patch tested?

Added tests to existing test suites.
After i applied these changes via reflection in my app OOM on driver side gone.

dongjoon-hyun · 2019-08-13T21:30:38Z

ok to test

streaming/src/test/scala/org/apache/spark/streaming/StreamingContextSuite.scala

core/src/main/scala/org/apache/spark/ui/SparkUI.scala

dongjoon-hyun · 2019-08-13T21:39:44Z

streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala

@@ -575,6 +577,8 @@ class StreamingContext private[streaming] (
          try {
            validate()

+            registerProgressListener()


I understand you are suggesting that the unregister is important, but do we need this? This seems to be a behavior change.

I think so. I believeStreamingTab shouldn't be responsible for registering/unregistering the listener as it could be and even already used in other place (metrics). Moreover seems there is also a bug that if ui is disabled, listener isn't registered and metrics aren't reported.

Yes I tend to agree. The behavior change is registering listeners when when the UI is disabled, but that seems like possibly a fix (we'd have to look deeper to figure out whether something else registers it in this case, like StreamingSource). This seems like a better place to manage it, certainly unregistering, and if it's unregistered here it should be registered here.

dongjoon-hyun · 2019-08-13T21:40:46Z

streaming/src/test/scala/org/apache/spark/streaming/InputStreamsSuite.scala

@@ -52,8 +52,6 @@ class InputStreamsSuite extends TestSuiteBase with BeforeAndAfter {

      // Set up the streaming context and input streams
      withStreamingContext(new StreamingContext(conf, batchDuration)) { ssc =>
-        ssc.addStreamingListener(ssc.progressListener)
-


If you remove the behavior change here( https://github.com/apache/spark/pull/25439/files#r313624949 ), it seems that we don't need to change this, right?

dongjoon-hyun · 2019-08-13T21:41:35Z

Thank you for your first contribution, @choojoyq . I left a few comments.

Hi, @zsxwing . Could you review this PR?

SparkQA · 2019-08-13T23:43:32Z

Test build #109063 has finished for PR 25439 at commit 2c55f89.

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

…JobProgressListener on stop

SparkQA · 2019-08-14T07:05:02Z

Test build #109088 has finished for PR 25439 at commit 4d5965e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

choojoyq · 2019-08-14T09:38:11Z

@dongjoon-hyun thanks for the so fast review.

dongjoon-hyun · 2019-08-16T17:16:22Z

Retest this please.

SparkQA · 2019-08-16T20:10:29Z

Test build #109232 has finished for PR 25439 at commit 4d5965e.

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

choojoyq · 2019-08-20T10:20:02Z

Hi @zsxwing, could you please take a look ?

srowen · 2019-08-20T14:00:27Z

streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala

@@ -575,6 +577,8 @@ class StreamingContext private[streaming] (
          try {
            validate()

+            registerProgressListener()


Yes I tend to agree. The behavior change is registering listeners when when the UI is disabled, but that seems like possibly a fix (we'd have to look deeper to figure out whether something else registers it in this case, like StreamingSource). This seems like a better place to manage it, certainly unregistering, and if it's unregistered here it should be registered here.

streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingTab.scala

srowen · 2019-08-23T13:59:29Z

core/src/main/scala/org/apache/spark/ui/WebUI.scala

@@ -93,6 +93,7 @@ private[spark] abstract class WebUI(
    attachHandler(renderJsonHandler)
    val handlers = pageToHandlers.getOrElseUpdate(page, ArrayBuffer[ServletContextHandler]())
    handlers += renderHandler
+    handlers += renderJsonHandler


Is this new change related?

Yes, handlers store reference to streaming and batch pages which store reference to StreamingJobProgressListener which stores reference to StreamingContext. So all the handlers should be unregistered.

I see so that's kind of an additional bug fix, that this never recorded the handler in the list?

I think so, seems that it wasn't an issue in past cause other tabs can't be detached in runtime.

dongjoon-hyun · 2019-08-24T16:19:17Z

Retest this please.

SparkQA · 2019-08-24T18:34:42Z

Test build #109670 has finished for PR 25439 at commit 4d5965e.

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

srowen · 2019-08-26T14:30:42Z

Merged to master

zzcclp · 2019-08-27T03:07:37Z

@choojoyq @srowen will this pr be merged into branch-2.4 too?

srowen · 2019-08-27T03:46:28Z

Yes I think that's OK. We're in a 'code freeze' for the 2.4.4 release at the moment, so I hesitate to merge anything but critical fixes until it's finalized. But it could go in for 2.4.5.

zzcclp · 2019-08-27T04:26:20Z

OK, thanks.

dongjoon-hyun · 2019-08-27T04:32:20Z

Thank you all. Yes. This is a good backport candidate for 2.4.5.

zzcclp · 2019-09-03T01:21:39Z

@choojoyq @srowen @dongjoon-hyun 2.4.4 was released, do you plan to merge this pr into branch-2.4?

In my application spark streaming is restarted programmatically by stopping StreamingContext without stopping of SparkContext and creating/starting a new one. I use it for automatic detection of Kafka topic/partition changes and automatic failover in case of non fatal exceptions. However i notice that after multiple restarts driver fails with OOM. During investigation of heap dump i figured out that StreamingContext object isn't cleared by GC after stopping. <img width="1901" alt="Screen Shot 2019-08-14 at 12 23 33" src="https://user-images.githubusercontent.com/13151161/63010149-83f4c200-be8e-11e9-9f48-12b6e97839f4.png"> There are several places which holds reference to it : 1. StreamingTab registers StreamingJobProgressListener which holds reference to Streaming Context directly to LiveListenerBus shared queue via ssc.sc.addSparkListener(listener) method invocation. However this listener isn't unregistered at stop method. 2. json handlers (/streaming/json and /streaming/batch/json) aren't unregistered in SparkUI, while they hold reference to StreamingJobProgressListener. Basically the same issue affects all the pages, i assume that renderJsonHandler should be added to pageToHandlers cache on attachPage method invocation in order to unregistered it as well on detachPage. 3. SparkUi holds reference to StreamingJobProgressListener in the corresponding local variable which isn't cleared after stopping of StreamingContext. Added tests to existing test suites. After i applied these changes via reflection in my app OOM on driver side gone. Closes #25439 from choojoyq/SPARK-28709-fix-streaming-context-leak-on-stop. Authored-by: Nikita Gorbachevsky <[email protected]> Signed-off-by: Sean Owen <[email protected]>

srowen · 2019-09-03T18:11:14Z

Also backported to 2.4. I resolved a minor merge conflict carefully and think I got it right.

zzcclp · 2019-09-04T02:56:06Z

Thank @srowen

… avoid CCE ### What changes were proposed in this pull request? [SPARK-27122](#24088) fixes `ClassCastException` at `yarn` module by introducing `DelegatingServletContextHandler`. Initially, this was discovered with JDK9+, but the class path issues affected JDK8 environment, too. After [SPARK-28709](#25439), I also hit the similar issue at `streaming` module. This PR aims to fix `streaming` module by adding `getContextPath` to `DelegatingServletContextHandler` and using it. ### Why are the changes needed? Currently, when we test `streaming` module independently, it fails like the following. ``` $ build/mvn test -pl streaming ... UISeleniumSuite: - attaching and detaching a Streaming tab *** FAILED *** java.lang.ClassCastException: org.sparkproject.jetty.servlet.ServletContextHandler cannot be cast to org.eclipse.jetty.servlet.ServletContextHandler ... Tests: succeeded 337, failed 1, canceled 0, ignored 1, pending 0 *** 1 TEST FAILED *** [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ ``` ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Pass the Jenkins with the modified tests. And do the following manually. Since you can observe this when you run `streaming` module test only (instead of running all), you need to install the changed `core` module and use it. ``` $ java -version openjdk version "1.8.0_222" OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_222-b10) OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.222-b10, mixed mode) $ build/mvn install -DskipTests $ build/mvn test -pl streaming ``` Closes #25791 from dongjoon-hyun/SPARK-29087. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

… avoid CCE ### What changes were proposed in this pull request? [SPARK-27122](#24088) fixes `ClassCastException` at `yarn` module by introducing `DelegatingServletContextHandler`. Initially, this was discovered with JDK9+, but the class path issues affected JDK8 environment, too. After [SPARK-28709](#25439), I also hit the similar issue at `streaming` module. This PR aims to fix `streaming` module by adding `getContextPath` to `DelegatingServletContextHandler` and using it. ### Why are the changes needed? Currently, when we test `streaming` module independently, it fails like the following. ``` $ build/mvn test -pl streaming ... UISeleniumSuite: - attaching and detaching a Streaming tab *** FAILED *** java.lang.ClassCastException: org.sparkproject.jetty.servlet.ServletContextHandler cannot be cast to org.eclipse.jetty.servlet.ServletContextHandler ... Tests: succeeded 337, failed 1, canceled 0, ignored 1, pending 0 *** 1 TEST FAILED *** [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ ``` ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Pass the Jenkins with the modified tests. And do the following manually. Since you can observe this when you run `streaming` module test only (instead of running all), you need to install the changed `core` module and use it. ``` $ java -version openjdk version "1.8.0_222" OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_222-b10) OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.222-b10, mixed mode) $ build/mvn install -DskipTests $ build/mvn test -pl streaming ``` Closes #25791 from dongjoon-hyun/SPARK-29087. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 729b318) Signed-off-by: Dongjoon Hyun <[email protected]>

In my application spark streaming is restarted programmatically by stopping StreamingContext without stopping of SparkContext and creating/starting a new one. I use it for automatic detection of Kafka topic/partition changes and automatic failover in case of non fatal exceptions. However i notice that after multiple restarts driver fails with OOM. During investigation of heap dump i figured out that StreamingContext object isn't cleared by GC after stopping. <img width="1901" alt="Screen Shot 2019-08-14 at 12 23 33" src="https://user-images.githubusercontent.com/13151161/63010149-83f4c200-be8e-11e9-9f48-12b6e97839f4.png"> There are several places which holds reference to it : 1. StreamingTab registers StreamingJobProgressListener which holds reference to Streaming Context directly to LiveListenerBus shared queue via ssc.sc.addSparkListener(listener) method invocation. However this listener isn't unregistered at stop method. 2. json handlers (/streaming/json and /streaming/batch/json) aren't unregistered in SparkUI, while they hold reference to StreamingJobProgressListener. Basically the same issue affects all the pages, i assume that renderJsonHandler should be added to pageToHandlers cache on attachPage method invocation in order to unregistered it as well on detachPage. 3. SparkUi holds reference to StreamingJobProgressListener in the corresponding local variable which isn't cleared after stopping of StreamingContext. Added tests to existing test suites. After i applied these changes via reflection in my app OOM on driver side gone. Closes apache#25439 from choojoyq/SPARK-28709-fix-streaming-context-leak-on-stop. Authored-by: Nikita Gorbachevsky <[email protected]> Signed-off-by: Sean Owen <[email protected]>

dongjoon-hyun added the DSTREAMS label Aug 13, 2019

dongjoon-hyun reviewed Aug 13, 2019

View reviewed changes

streaming/src/test/scala/org/apache/spark/streaming/StreamingContextSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Aug 13, 2019

View reviewed changes

streaming/src/test/scala/org/apache/spark/streaming/StreamingContextSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Aug 13, 2019

View reviewed changes

core/src/main/scala/org/apache/spark/ui/SparkUI.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Aug 13, 2019

View reviewed changes

choojoyq force-pushed the SPARK-28709-fix-streaming-context-leak-on-stop branch 2 times, most recently from 75935cd to fbfabb0 Compare August 14, 2019 06:31

[SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…

4d5965e

…JobProgressListener on stop

choojoyq force-pushed the SPARK-28709-fix-streaming-context-leak-on-stop branch from fbfabb0 to 4d5965e Compare August 14, 2019 06:33

srowen reviewed Aug 20, 2019

View reviewed changes

srowen reviewed Aug 23, 2019

View reviewed changes

srowen approved these changes Aug 23, 2019

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…~~ [SPARK-28709][DSTREAMS] Fix StreamingContext leak through Streaming Aug 24, 2019

srowen closed this in 9f8c7a2 Aug 26, 2019

dongjoon-hyun mentioned this pull request Sep 15, 2019

[SPARK-29087][CORE][STREAMING] Use DelegatingServletContextHandler to avoid CCE #25791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-28709][DSTREAMS] Fix StreamingContext leak through Streaming #25439

[SPARK-28709][DSTREAMS] Fix StreamingContext leak through Streaming #25439

choojoyq commented Aug 13, 2019 •

edited

Loading

dongjoon-hyun commented Aug 13, 2019

dongjoon-hyun Aug 13, 2019

choojoyq Aug 14, 2019

srowen Aug 20, 2019

dongjoon-hyun Aug 13, 2019 •

edited

Loading

choojoyq Aug 14, 2019

dongjoon-hyun commented Aug 13, 2019

SparkQA commented Aug 13, 2019

SparkQA commented Aug 14, 2019

choojoyq commented Aug 14, 2019

dongjoon-hyun commented Aug 16, 2019

SparkQA commented Aug 16, 2019

choojoyq commented Aug 20, 2019

srowen Aug 20, 2019

srowen Aug 23, 2019

choojoyq Aug 23, 2019

srowen Aug 23, 2019

choojoyq Aug 24, 2019

dongjoon-hyun commented Aug 24, 2019

SparkQA commented Aug 24, 2019

srowen commented Aug 26, 2019

zzcclp commented Aug 27, 2019

srowen commented Aug 27, 2019

zzcclp commented Aug 27, 2019

dongjoon-hyun commented Aug 27, 2019

zzcclp commented Sep 3, 2019

srowen commented Sep 3, 2019

zzcclp commented Sep 4, 2019

[SPARK-28709][DSTREAMS] Fix StreamingContext leak through Streaming #25439

[SPARK-28709][DSTREAMS] Fix StreamingContext leak through Streaming #25439

Conversation

choojoyq commented Aug 13, 2019 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

dongjoon-hyun commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Aug 13, 2019

SparkQA commented Aug 13, 2019

SparkQA commented Aug 14, 2019

choojoyq commented Aug 14, 2019

dongjoon-hyun commented Aug 16, 2019

SparkQA commented Aug 16, 2019

choojoyq commented Aug 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Aug 24, 2019

SparkQA commented Aug 24, 2019

srowen commented Aug 26, 2019

zzcclp commented Aug 27, 2019

srowen commented Aug 27, 2019

zzcclp commented Aug 27, 2019

dongjoon-hyun commented Aug 27, 2019

zzcclp commented Sep 3, 2019

srowen commented Sep 3, 2019

zzcclp commented Sep 4, 2019

choojoyq commented Aug 13, 2019 •

edited

Loading

dongjoon-hyun Aug 13, 2019 •

edited

Loading