[SPARK-2435] Add shutdown hook to pyspark

[mattf]

No description provided.

[SparkQA]

QA tests have started for PR 2183 at commit ee0ee99.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2183 at commit ee0ee99.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[davies]

What's the problem without this patch? I remember that the JVM will shutdown itself after shell exited.

[JoshRosen]

I think the idea is to perform a more graceful shutdown of SparkContext. Do we have similar shutdown hooks in Java / Scala?

[andrewor14]

In the Scala shell if you do Ctrl+D it stops the SparkContext. In Python I think there is no way to stop the SparkContext unless you call sc.stop().

[davies]

Is it better to put atexit.register() in context.py? So all the pyspark jobs can have this.

[JoshRosen]

It looks like Spark core automatically registers JVM shutdown hooks for several different components, so it might okay to have the similar global logic here.

One (perhaps minor) concern with the global hook is that the functions we register will contain strong references to the SparkContext, which might lead to resource leaks in a long-running Python process that creates and destroys many contexts.

PySpark does not currently support running multiple SparkContexts at the same time, so one option would be to define a single shutdown hook that stops SparkContext._active_spark_context. There's currently a lock (SparkContext._lock) guarding that field and I'm not sure whether it's safe to attempt to acquire it during a shutdown hook (it's fine for shutdown hooks to throw exceptions, but they shouldn't block). To guard against this, maybe we can attempt to acquire the lock and just throw an exception after a short timeout. This is a super-rare edge case, though, and I'd be shocked if anyone ran into it, since it requires a separate thread attempting to start or stop a SparkContext while the Python interpreter is exiting.

[mattf]

What's the problem without this patch? I remember that the JVM will shutdown itself after shell exited.

davies, i went back and tried to reproduce the shell issues (while scripting the shell) and couldn't reproduce w/ or w/o this patch. so i don't have a bug to justify the patch. apologies.

[mattf]

Is it better to put atexit.register() in context.py? So all the pyspark jobs can have this.

i think it's a question of who owns the context. the owner is whomever constructed it. they should be responsible for stopping it, and given the open not to stop it if they so desire.

in the pyspark shell, it's the shell.py that creates it and should therefore stop it.

[JoshRosen]

This seems like a good fix, if only for consistency with bin/spark-shell, so I'm going to merge this into master. Thanks!