Qtconsole fix racecondition #1065

Merged
merged 4 commits into from Nov 30, 2011

Projects

None yet

3 participants

@Carreau
Member
Carreau commented Nov 29, 2011

See deee340f for descripition, 4573b16 just revert @fperez 65546bf which was temporary.

fix race condition in qtconsole due to a race condition beween the population
of the magic menu and the first prompt request. Resolved by upgrading the
logic of the console to handle several executions request in parallel.

Some namedTuple might still carry redundent information but the fix is the
first priority

Fixes #1057 , introduced by #956.

Note that I can't reproduce the bug, so please test carefully as it changes the logic of exec reply handeling in qtconsole

@Carreau Carreau Revert "Temporary fix to work around #1057."
    This reverts commit 65546bf, done to
    temporaly fixed a race condition introduced by #956, next commits should
    fixe this race condition
4573b16
@Carreau
Member
Carreau commented Nov 29, 2011

forgot one case... close to reopen later... sorry

@Carreau Carreau closed this Nov 29, 2011
@Carreau Carreau qtconsole: fix race-cond, handle multiple exec
	fix race condition in qtconsole due to a race condition beween the population
	of the magic menu and the first prompt request. Resolved by upgrading the
	logic of the console to handle several executions request in parallel.

	Some namedTuple might still carry redundent information but the fix is the
	first priority

	fixes #1057 , introduced by #956
8553345
@Carreau Carreau reopened this Nov 29, 2011
@Carreau
Member
Carreau commented Nov 29, 2011

Fixed and rebased... not my day...
Again, this fixes qtconsole race condition but changes the logic in qtconsole. Please test carefully before merging.
New Head is 8553345, github seems to not see it (yet ?)

@fperez
Member
fperez commented Nov 29, 2011

I've tested it interactively and it looks good so far, I'll have another round of testing at home tonight on the computer where I saw the problem most reproducibly. I also want to wait for feedback from @minrk and @takluyver who were already participating on #1057, but it looks like we're close to a solution. Thanks for the work!

@fperez
Member
fperez commented Nov 29, 2011

Code looks clean and logic OK to me. Unless the others have any objection, once we test it a bit more we can merge.

@minrk minrk commented on an outdated diff Nov 29, 2011
IPython/frontend/qt/console/frontend_widget.py
@@ -373,12 +374,14 @@ class FrontendWidget(HistoryConsoleWidget, BaseFrontendMixin):
""" Handles replies for code execution.
"""
self.log.debug("execute: %s", msg.get('content', ''))
- info = self._request_info.get('execute')
+ info_list = self._request_info.get('execute')
+ msg_id = msg['parent_header']['msg_id']
+ if msg_id in info_list:
+ info = info_list[msg_id]
@minrk
minrk Nov 29, 2011 IPython member

You don't have to be protective with _request_info.get('execute'), because that should be guaranteed to exist. This can be reduced to:

msg_id = msg['parent_header'].get('msg_id')
info = self._request_info['execute'].get(msg_id)

which will also fix the bug in the following line, where info would not be defined if the msg_id was not found.

@minrk minrk commented on an outdated diff Nov 29, 2011
IPython/frontend/qt/console/history_console_widget.py
def _handle_execute_reply(self, msg):
""" Handles replies for code execution, here only session history length
"""
- info = self._request_info.get('execute')
- if info and info.id == msg['parent_header']['msg_id'] and \
- info.kind == 'save_magic' and not self._hidden:
- content = msg['content']
- status = content['status']
- if status == 'ok':
- self._max_session_history=(int(content['user_expressions']['hlen']))
+ info_list = self._request_info.get('execute')
@minrk
minrk Nov 29, 2011 IPython member

probably change info_list to info_dict here and elsewhere, since it's not a list anymore

@minrk minrk commented on an outdated diff Nov 29, 2011
IPython/frontend/qt/console/history_console_widget.py
def _handle_execute_reply(self, msg):
""" Handles replies for code execution, here only session history length
"""
- info = self._request_info.get('execute')
- if info and info.id == msg['parent_header']['msg_id'] and \
- info.kind == 'save_magic' and not self._hidden:
- content = msg['content']
- status = content['status']
- if status == 'ok':
- self._max_session_history=(int(content['user_expressions']['hlen']))
+ info_list = self._request_info.get('execute')
+ msg_id = msg['parent_header']['msg_id']
@minrk
minrk Nov 29, 2011 IPython member

same as before:

info = self._request_info['execute'].pop(msg_id, None)
@minrk
Member
minrk commented Nov 29, 2011

Looks good, with the few small changes I noted inline. Thanks!

@Carreau
Member
Carreau commented Nov 29, 2011

@minrk
I guess you're right, thanks, fixed.

@fperez
Member
fperez commented Nov 29, 2011

Great, thanks. I'll do a final round of testing tonight at home where the problem was more clearly visible, and will merge if it looks OK.

@fperez
Member
fperez commented Nov 30, 2011

OK, I've verified here that it works fine. So we're good to go, thanks! Merging now.

@fperez fperez merged commit a5fd0a3 into ipython:master Nov 30, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment