issue440: input system use of coroutines, asynchronous prefix commands

Priority: bug Status: chatting
Messages
msg1108 (view) Author: jbms Date: 2013-02-05.08:34:43
There seem to be several bugs due to the way the input system is using coroutines.

- As a side note, the system seems designed to allow multiple nested in-progress key sequences, which
makes things more complicated and more difficult to understand.  As far as I can tell, this is only
needed to support using prefix commands with M-x.  Maybe the added complexity is nonetheless worth it,
though.

- I haven't looked at the code to fully understand how everything interacts with
window.input.begin_recursion()/end_recursion(), but I believe it may need to be added even for
minibuffer_message_state, as for instance minibuffer.show_wait_message (used by minibuffer.wait_for, in
turn used by some invocations of an external editor, for example) depends on having a different key map
working.  It is also not totally clear if these should be called in load/unload rather than in the
constructor/destroy.

- When running normal commands, input_handle_sequence, which is itself a long-running coroutine, invokes
the commands as part of its own coroutine using "yield call_interactively(...)".  This is a problem
because the command may indeed make use of the coroutine, and take arbitrarily long, waiting for external
prorgams, for things to download, etc.  Meanwhile, when the interactive command is waiting on something
internally to finish, a key event handlers like input_handle_keypress, etc. might be invoked by Mozilla,
and since the continuation is still active, will unexpectedly wake up the coroutine with a key event
object.  For example, some other random function like in spawn-process.js will have done "var x = yield
SUSPEND;" and expect to be woken up by some internal mechanism with some appropriate value or exception.
 Instead, though, it gets x assigned to a key event object that it isn't expecting (and that key event
object never makes it to input_handle_sequence where it belongs).  In short, if a key event happens while
the coroutine is still being used by the command, there will be undefined behavior.  

  A specific case where this breaks something is with edit-current-field-in-external-editor.

  The most direct solution is to instead invoke the command in a new "fire-and-forget" coroutine using
e.g. co_call(call_interactively(...)).

- Prefix commands present a special problem because the above solution doesn't quite work.  The prefix
command is supposed to affect the current key sequence context, so we can't just fire it off
asynchronously and proceed without waiting for it to be done.  However, it isn't clear what should happen
if a key event comes in while we are waiting for a prefix command to finish running.  Probably the answer
would have to be either ignoring it, or queuing it up somehow.  Either way it feels like an unnatural
thing to do.  The situation is significantly simplified if prefix commands are required to be regular,
synchronous functions and cannot be coroutine functions, as the hook functions that they replaced were. 
In that case, we can safely run the prefix command immediately and not have to worry about what to do
while waiting for it to finish.  Indeed, all of the prefix commands in conkeror are regular synchronous
functions, with one exception: execute-extended-command.  In practice, this works because
execute-extended-command calls minibuffer.read_command, which calls window.input.begin_recursion, which
prevents the input system from clobbering the existing continuation being used by
execute-extended-command.  That is also why call_after_timeout is needed later, as otherwise
input_handle_command would attempt the invalid operation of waking up the already-running continuation. 
Of course this does mean there is a small window during which a random other key could be pressed and get
processed before input_handle_command is called, though I don't think that is necessarily a practical
problem.  I think there might be an elegant solution that does not require nested active key sequences,
though I haven't thought through all of the details: maybe having execute-extended-command store the
current input state and then tell the input system to use it once it finishes running the specified command.
msg1109 (view) Author: jbms Date: 2013-02-08.09:57:16
I pushed a fix to the jbms-input-system-fix branch.  Note: this did involve significant reorganization of
input.js but I don't believe any functionality is lost and impact on users should be minimal.  I think
overall the code is a bit simpler though code length may have increased due to added comments.  Probably
any user keypress_hook code will need to be changed, but that is kind of unavoidable.  I changed
call_interactively to not be a coroutine, though this won't necessarily break any user code, since the
existing call_interactively ate any exceptions that were thrown anyway.  Instead, run_interactively is
better to use if you want to wait for it to finish, since then you can also handle any exceptions thrown.

Please let me know if this introduces any breakage or has other problems.
msg1110 (view) Author: retroj Date: 2013-02-09.04:36:14
I've not yet reviewed the patch, but regarding edit-current-field-in-external-editor, wouldn't
it solve the loss-of-state problem neatly to simply to start the external editor in a new
coroutine instead of in the input_handle_sequence coroutine?
msg1111 (view) Author: jbms Date: 2013-02-09.08:06:54
Well, that is basically what needs to happen, but in fact it is not just that command that should be run
in a separate coroutine, but rather all commands, except for prefix commands which need to be dealt with
a bit differently.  And that is basically what I try to do in the patch.  I think if just that one
command were special-cased, there would likely still remain some cases where there is a problem.  Using
yield SUSPEND/yield CONTINUATION requires that the code calling yield SUSPEND be tightly coordinated with
the code that uses the continuation to wake it up.  This could be ensured by setting state.continuation
only when the input system has in fact called yield SUSPEND.  That is pretty much the basis of my patch,
just that it then seemed simpler to no longer use a coroutine at all for handling input events.
History
Date User Action Args
2013-02-09 08:06:54jbmssetmessages: + msg1111
2013-02-09 04:36:15retrojsetmessages: + msg1110
2013-02-08 09:57:17jbmssetstatus: unread -> chatting
messages: + msg1109
2013-02-05 08:34:43jbmscreate