python: Fix possible missing events after SPEAK command #781
Conversation
| self._callback_handler.begin_add_callback() | ||
| try: | ||
| self._conn.send_command('SPEAK') | ||
| result = self._conn.send_data(text) | ||
| if callback: | ||
| msg_id = int(result[2][0]) | ||
| except: | ||
| if callback: | ||
| self._callback_handler.cancel_add_callback() | ||
| raise | ||
| else: | ||
| if callback: | ||
| self._callback_handler.end_add_callback(msg_id, callback, event_types) |
There was a problem hiding this comment.
This is a bit convoluted and I would have loved providing a simpler API, maybe like so (100% untested):
def add_callback(self, callback=None, event_types=None):
"""
with handler.add_callback(callback, event_types) as request:
connection.send_command('SPEAK')
result = connection.send_data(data)
request.set_message_id(int(result[2][0]))
"""
class AddCallbackRequest:
def __init__(self, handler, callback=None, event_types=None):
self._handler = handler
self._msg_id = None
self._callback = callback
self._event_types = event_types
def set_message_id(self, msg_id):
self._msg_id = msg_id
def __enter__(self):
self.handler.begin_add_callback()
return self
def __exit__(self, exc_type, exc_value, traceback):
if exc_type:
self.handler.cancel_add_callback()
else:
assert self._msg_id is not None
self.handler.end_add_callback(self._msg_id, self._callback, self._event_types)
return False
return AddCallbackRequest(self, callback, event_types)but unfortunately as this (sole) user has a conditional callback it's not really usable. Meh.
There was a problem hiding this comment.
I would be fine with something like this:
if callback:
with handler.add_callback(callback, event_types) as request:
self._conn.send_command('SPEAK')
result = self._conn.send_data(text)
request.set_message_id(int(result[2][0]))
else:
self._conn.send_command('SPEAK')
result = self._conn.send_data(text)
Probably just nobody had a look and it wasn't hurting people enough for anybody to take up the task.
I'm quite afraid of this. There is the deadlock issue indeed, but also events could then be handled mixed up, if some events come while I'm thinking that we may just not need the I'd then much rather see the communication thread always do the processing, which I believe is not that complex: on This means we're moving the concurrency management from |
I'm not quite sure how could this happen?
Well, this is not trivial, because if we do not want to delay callbacks too much, we need not to wait for the next message to be received before releasing possibly held off callbacks. And this means making the whole Another option which would make the code simpler (yet sound a tad crazy) would be to have an additional thread only responsible of managing callbacks, decoupling IO and callbacks. I'm not sure this is sound, so I'd rather have your opinion here. Or maybe I'm completely missing something? |
The execution will be synchronized, yes, but can happen in whatever order depending on the locking order, I don't think we want that, and rather see callbacks always called in the event order.
Do we actually care? The command is non-blocking, so we'll only have the round-trip with the server, which should be fast enough.
There is a saying that adding a thread most often only adds more headaches than solves things :) The problem is that quite often we do need to have proper ordering between requests from the application and events, so that events don't get handled before/after requests that have some interactions with them. Having a single loop that processes everything makes it way simpler to achieve it. |
Well, if we hold off all callbacks while an addition is in the works, that wouldn't happen: either diff --git a/src/api/python/speechd/client.py b/src/api/python/speechd/client.py
index 3743af87..97ab1ff4 100644
--- a/src/api/python/speechd/client.py
+++ b/src/api/python/speechd/client.py
@@ -439,18 +439,19 @@ class _CallbackHandler(object):
return
self._lock.acquire()
try:
- try:
- callback, event_types = self._callbacks[msg_id]
- except KeyError:
- # if we don't have a handler for that message but we have
- # pending requests, queue the message for possible later use
- if self._pending_adds:
- if msg_id not in self._pending_events:
- self._pending_events[msg_id] = []
- self._pending_events[msg_id].append((type, kwargs))
+ # if we have pending requests, queue the message for possible later use
+ if self._pending_adds:
+ if msg_id not in self._pending_events:
+ self._pending_events[msg_id] = []
+ self._pending_events[msg_id].append((type, kwargs))
else:
- if self._handle_event(callback, event_types, type, kwargs):
- del self._callbacks[msg_id]
+ try:
+ callback, event_types = self._callbacks[msg_id]
+ except KeyError:
+ pass
+ else:
+ if self._handle_event(callback, event_types, type, kwargs):
+ del self._callbacks[msg_id]
finally:
self._lock.release()
But what if all events have been received by the time the caller issues self._handler.being_add_callback()
# here we start piling up callbacks...
self._conn.send_command('SPEAK')
result = self._conn.send_data(text)
time.sleep(60) # or any arbitrary delay happening in this thread
self._handler.end_add_callback(int(result[2][0]), callback, event_types)
# and now, how are the queued callbacks dispatched?
# point is, everything is piled up and we won't get any more activity in
# the communication thread until we issue another command |
This fixes the very old (before early 2007 at least) TODO not to miss events in the Python client implementation if they come in too quickly for having set up the listener properly.
As I didn't see a trivial fix short of predicting the message ID (is that possible? that would be a good solution :)) -- and I'd wager there probably isn't if it's gone known and unfixed for so long -- I implemented a more complex one.
What I do here is start queuing events before actually knowing the message ID, and when I do finally know it, dispatch any possibly caught event, and then attach the normal callback. This requires care in the caller side, but as it's an internal API only used in one place it's likely fine.
There is one "big" user-facing change (apart from not missing events): collected events are dispatched directly from within
speak(), and not from the communication thread. I would think this is likely OK because of the restrictions the current callbacks already have, but in theory it could break a caller that holds a lock when calling speak() and use that same lock also in the callback (that would deadlock).As for testing, I didn't see any Python API tests yet so I didn't really know what to do, but I verified both with Orca and with a dummy client like so:
say.py
input.ssml
Yeah, this is an excerpt from an Orca debug log, you guessed it ;)
Calling this
Note that somehow I reproduce this a lot more easily in 0.10 than 0.11, no real idea why. With 0.10 it basically always fails on the second sentence (I'd guess because by then spd is already set up and running, so snappier?), whereas on 0.11 it's a lot more random, and often works with or without it.