Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block until kernel termination after sending a kill signal #2369

Merged
merged 1 commit into from Sep 27, 2012
Merged

Block until kernel termination after sending a kill signal #2369

merged 1 commit into from Sep 27, 2012

Conversation

epatters
Copy link
Contributor

As users of the Qt console have probably noticed, restarting the kernel is not as reliable as it ought to be. In some cases, a second restart seems to fix the problem; in other cases, the frontend itself becomes wedged. I am currently investigating these problems.

In trying to reproduce these problems, one thing that I noticed immediately is that restarting the kernel via a kill (rather than a clean shutdown) often fails, at least on OS X. Try this:

  1. Change now=False to now=True in https://github.com/ipython/ipython/blob/master/IPython/frontend/qt/console/frontend_widget.py#L273
  2. Attempt to restart the kernel (Ctrl+.)

Usually, I get the following traceback in the frontend process:

Traceback (most recent call last):
  File "/Users/epatters/Local/IPython/IPython/frontend/qt/console/mainwindow.py", line 836, in restart_kernel_active_frontend
    self.active_frontend.request_restart_kernel()
  File "/Users/epatters/Local/IPython/IPython/frontend/qt/console/frontend_widget.py", line 273, in request_restart_kernel
    self.restart_kernel(message, now=False)
  File "/Users/epatters/Local/IPython/IPython/frontend/qt/console/frontend_widget.py", line 635, in restart_kernel
    self.kernel_manager.restart_kernel(now=now)
  File "/Users/epatters/Local/IPython/IPython/zmq/kernelmanager.py", line 882, in restart_kernel
    self.start_kernel(**self._launch_args)
  File "/Users/epatters/Local/IPython/IPython/frontend/qt/kernelmanager.py", line 211, in start_kernel
    super(QtKernelManager, self).start_kernel(*args, **kw)
  File "/Users/epatters/Local/IPython/IPython/zmq/kernelmanager.py", line 812, in start_kernel
    self.kernel = launch_kernel(fname=self.connection_file, **kw)
  File "/Users/epatters/Local/IPython/IPython/zmq/ipkernel.py", line 878, in launch_kernel
    *args, **kwargs)
  File "/Users/epatters/Local/IPython/IPython/zmq/entry_point.py", line 200, in base_launch_kernel
    stdin=_stdin, stdout=_stdout, stderr=_stderr, cwd=cwd)
  File "/Library/Frameworks/EPD64.framework/Versions/7.3/lib/python2.7/subprocess.py", line 679, in __init__
    errread, errwrite)
  File "/Library/Frameworks/EPD64.framework/Versions/7.3/lib/python2.7/subprocess.py", line 1234, in _execute_child
    data = _eintr_retry_call(os.read, errpipe_read, 1048576)
  File "/Library/Frameworks/EPD64.framework/Versions/7.3/lib/python2.7/subprocess.py", line 478, in _eintr_retry_call
    return func(*args)
OSError: [Errno 22] Invalid argument

Now, this error is quite obscure and I don't really understand what is going on here, but I think the problem is that the new kernel is sometimes started before the old kernel has actually terminated. In any event, waiting for the kernel to terminate before continuing seems to fix the problem.

My only concern with this PR is that blocking calls without timeouts make me a little nervous. If, for some reason, the kernel survives SIGKILL, the frontend application will deadlock. Is this a reasonable concern in practice?

@travisbot
Copy link

This pull request passes (merged 0cee7c56 into 0af1e9d).

@fperez
Copy link
Member

fperez commented Sep 24, 2012

Technically, there's no way for a process to survive a SIGKILL short of something going zombie in the OS kernel itself. SIGKILL is an un-ignorable, un-handlable signal: the OS won't let you even try to install a handler for it. So if a process survives a SIGKILL it's because the OS kernel couldn't kill it, at that point something is probably really wrong with the system and I'm not sure worrying about the frontend will help much anways.

@epatters
Copy link
Contributor Author

Thanks, @fperez, for that useful information. Given that SIGKILL cannot fail under any reasonable circumstances, I think we should merge this. It would nice, though, if another OS X user could confirm the bug and the fix.

@fperez
Copy link
Member

fperez commented Sep 25, 2012

Also, do you know what this does on Windows, or can someone at Enthought test? All I said was posix-specific, the world of signals in windows is a dark unknown to me.

@epatters
Copy link
Contributor Author

According to MSDN (http://msdn.microsoft.com/en-us/library/windows/desktop/ms686714(v=vs.85).aspx), TerminateProcess guarantees that "a process cannot prevent itself from being terminated." So this ought to be safe on Windows as well.

@@ -894,13 +894,15 @@ def has_kernel(self):
return self.kernel is not None

def kill_kernel(self):
""" Kill the running kernel. """
""" Kill the running kernel. This method blocks until the kernel process
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial note; let's keep the format of docstrings (I'm sure we violate it somewhere, just trying to keep things tidy where we see it) to

Single-line description
<BLANK LINE>
rest of text...
...
...

So this would simply become

Kill the running kernel.

This method blocks until...

The reason is that we can have popups and other tools in the future that summarize things by culling only the first line; for that to work well, the first line must be a complete sentence and not truncate.

Same reason why that approach is the recommended way to write git commit messages (minus the 50-char limit).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I really should know better.

@fperez
Copy link
Member

fperez commented Sep 27, 2012

Trivial docstring fix, then we can merge. Thanks!!

@epatters
Copy link
Contributor Author

Cleaned up the docstrings.

@fperez
Copy link
Member

fperez commented Sep 27, 2012

Great, merging now, thanks!

fperez added a commit that referenced this pull request Sep 27, 2012
Block until kernel termination after sending a kill signal.  This will make for more robust kernel restarting in the Qt console.
@fperez fperez merged commit 0e7b3a2 into ipython:master Sep 27, 2012
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Block until kernel termination after sending a kill signal.  This will make for more robust kernel restarting in the Qt console.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants