adhearsion / punchblock

Telephony middleware library for Ruby
adhearsion.com/punchblock
MIT License
40 stars 34 forks source link

Message::Say.new not returning on a long play of a URL #10

Closed jsgoecke closed 13 years ago

jsgoecke commented 13 years ago

If I understand the scenario correctly, the Message::Say.new 'http://foobar.com', :url should return an object that I may then do something like:

say = @protocol::Message::Say.new 'http://foobar.com', :url say.pause say.resume

The issue is, the Say does not return an object before playback is over so that I may act upon it. Here is the wire and transport log:

https://gist.github.com/d4da52b914cb00d22c9f

bklang commented 13 years ago

Not quite. The outgoing Say message is one-way. You send it to the Transport layer and it goes off into the aether. When that Say command starts executing, the Ozone server should send a message back down the Transport layer which gets turned into a brand new Say object. This should be (but probably is not yet) the return value of Transport#write. It is this object that is returned that contains the cmd_id which is necessary for generating the response events such as #pause and #resume. We just have not gotten that far in the testing yet.

jsgoecke commented 13 years ago

Yeah, sorry, was thinking in my synchronous DSLish way. What I mean, is that an event should return that I may read off of the queue and get the object I need to execute the control events on (pause, resume, stop, etc).

benlangfeld commented 13 years ago

What we have now, is #pause!, #resume! methods on Say. These rely on the Say object knowing its command ID, which it does not until we get an IQ result containing a <ref/>. Fortunately, Ozone::Connection#write blocks until we get this back, and in this process the command_id is tagged on the Say. Thus, if everything is working correctly, once write returns you should be able to call #pause! on your original Say object.

Of course, there are several failure modes:

  1. No result is received within the 3 second limit that Connection#write will block for. Currently, this results in the string representation of a Timeout::Error being written to the queue, which fails miserably. I suspect what we really want to do here is raise either a TransportError or a ProtocolError (most likely the former) with a message stating that no response was received.
  2. The response returned is an Ozone error, in which case we raise a ProtocolError which provides some meaningful information about the failure. This would then be the responsibility of the DSL layer to rescue cleanly.
  3. We try to call #pause! on a Say that is completed or does not know its command ID. Currently, Punchblock does not protect against this. The latter is easily solved. The former would require each Say object to know about all of its events, and as such track its own state. Is this something we want Punchblock to be responsible for, or shift it further up?
bklang commented 13 years ago

In the case of #1 above, we need to add a mechanism to cancel the original command. If XMPP has guaranteed delivery and we do not get a response back in 3 seconds, it's likely the command is queued somewhere. Also: is 3 seconds sufficient?

As for #3, I think that the Say object should track its own state. There are too many backend-specific details that are relevant, which is exactly the kind of thing we want to hide from the frameworks.

benlangfeld commented 13 years ago

XMPP does not include guaranteed delivery in the core spec, though it is the case that an IQ MUST respond immediately. Any command queueing should not cause a delay to an IQ response. As such, if we're following that spec, any delay more than x should be considered as failed delivery of some variety.

benlangfeld commented 13 years ago

I'll look into commands tracking their own state.

benlangfeld commented 13 years ago

Commands now track their own state (they know when they are new, requested, executing, paused, stopped, etc) and their actions (#stop!, #pause!, etc) now execute directly (they do not return a or element) since they know which call/command id and connection to use from their parent. I think that covers that concern.

As for timeouts, see https://github.com/tropo/punchblock/issues/50

Please re-open if I have not covered this fully.