Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Don't assume history requests succeed in qtconsole #745

Merged
merged 3 commits into from

2 participants

Min RK Evan Patterson
Min RK
Owner

If there was an error in user startup code, the history request will be aborted. It is generally not acceptable in frontends to assume that requests succeed. This assumption is made all over our frontend code, and should be be fixed. For now, this handles a known issue of the qtconsole crashing hard when invalid input is given to startup code (e.g. ipython qtconsole -c "DNE").

If the history request was aborted (this means a different request raised an error, causing the queue to be flushed), it will be tried again, exactly once, after waiting for the abort flush.

minrk added some commits
Min RK minrk make ConsoleWidget LoggingConfigurable
This adds the attribute Widget.log, which is hooked up to the
running application's logger.
b13dbdb
Min RK minrk Don't assume history request succeeded
errors in startup can cause the history request to be aborted.  There could be any of
a number of reasons the history request should fail, and the frontend should *never*
assume that the status is 'ok'.
24d3b02
Min RK minrk retry aborted history requests in qtconsole fc67794
Evan Patterson
Collaborator

Thanks for the fix, Min. I noticed this crash recently myself.

That said, this approach makes me a little leery. The Qt console (and no doubt other frontends as well) makes many different requests: for execution, history, tab completion, object info, etc. It seems painful to have check status and possibly retry the request under all these differences circumstances. I wonder if it possible to abstract at least some of this away, e.g. by having BaseFrontendMixin handle the retries automatically.

This is all somewhat vague. I will think about this some more and see if I can produce anything more substantial.

Evan Patterson
Collaborator

OK, I have an implementation plan for more robust error handling. Since this will involve some refactoring, I think it's worth merging this now as an interim fix.

Min RK minrk merged commit 17494a1 into from
Brian E. Granger ellisonbg referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 29, 2011
  1. Min RK

    make ConsoleWidget LoggingConfigurable

    minrk authored
    This adds the attribute Widget.log, which is hooked up to the
    running application's logger.
  2. Min RK

    Don't assume history request succeeded

    minrk authored
    errors in startup can cause the history request to be aborted.  There could be any of
    a number of reasons the history request should fail, and the frontend should *never*
    assume that the status is 'ok'.
  3. Min RK
This page is out of date. Refresh to see the latest.
6 IPython/frontend/qt/console/console_widget.py
View
@@ -16,7 +16,7 @@
from IPython.external.qt import QtCore, QtGui
# Local imports
-from IPython.config.configurable import Configurable
+from IPython.config.configurable import LoggingConfigurable
from IPython.frontend.qt.rich_text import HtmlExporter
from IPython.frontend.qt.util import MetaQObjectHasTraits, get_font
from IPython.utils.text import columnize
@@ -39,7 +39,7 @@ def is_letter_or_number(char):
# Classes
#-----------------------------------------------------------------------------
-class ConsoleWidget(Configurable, QtGui.QWidget):
+class ConsoleWidget(LoggingConfigurable, QtGui.QWidget):
""" An abstract base class for console-type widgets. This class has
functionality for:
@@ -170,7 +170,7 @@ def __init__(self, parent=None, **kw):
The parent for this widget.
"""
QtGui.QWidget.__init__(self, parent)
- Configurable.__init__(self, **kw)
+ LoggingConfigurable.__init__(self, **kw)
# Create the layout and underlying text widget.
layout = QtGui.QStackedLayout(self)
22 IPython/frontend/qt/console/ipython_widget.py
View
@@ -12,6 +12,7 @@
import re
from subprocess import Popen
import sys
+import time
from textwrap import dedent
# System library imports
@@ -104,6 +105,7 @@ class IPythonWidget(FrontendWidget):
_payload_source_exit = zmq_shell_source + '.ask_exit'
_payload_source_next_input = zmq_shell_source + '.set_next_input'
_payload_source_page = 'IPython.zmq.page.page'
+ _retrying_history_request = False
#---------------------------------------------------------------------------
# 'object' interface
@@ -174,7 +176,25 @@ def _handle_history_reply(self, msg):
""" Implemented to handle history tail replies, which are only supported
by the IPython kernel.
"""
- history_items = msg['content']['history']
+ content = msg['content']
+ if 'history' not in content:
+ self.log.error("History request failed: %r"%content)
+ if content.get('status', '') == 'aborted' and \
+ not self._retrying_history_request:
+ # a *different* action caused this request to be aborted, so
+ # we should try again.
+ self.log.error("Retrying aborted history request")
+ # prevent multiple retries of aborted requests:
+ self._retrying_history_request = True
+ # wait out the kernel's queue flush, which is currently timed at 0.1s
+ time.sleep(0.25)
+ self.kernel_manager.shell_channel.history(hist_access_type='tail',n=1000)
+ else:
+ self._retrying_history_request = False
+ return
+ # reset retry flag
+ self._retrying_history_request = False
+ history_items = content['history']
items = [ line.rstrip() for _, _, line in history_items ]
self._set_history(items)
Something went wrong with that request. Please try again.