Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Allow setting the ActionID on an Action message. #31

Merged
merged 13 commits into from

2 participants

@jacobkiers

UPDATE 2012-07-12 16:18: Updated the example
Hi Marcelo,

I have added the possibility to override the ActionID, including tests. The default behaviour (automatically generating it with microtime), has not changed.

I would appriciate it if you pulled these changes.

Rationale
Only the thread sending the Originate knows the ActionID it was sent with. Since the default behaviour is using microtime(true), this means that only that thread can matche the OriginateResponseEvent to the corresponding call. By adding the ability to set the ActionID, another process can handle AMI messages, and still know to which call an OriginateResponseEvent belongs.

This gives the possibility to separate call setup with response handling.

Example
Let's say we have 2 processes: the first is a Dialer, a very dumb process, just sending OriginateActions. The other one is a Reporter, handling all aspects of reporting calls, like custom handling of HangupEvent and OriginateResponseEvent messages.

The Dialer (process 1) sends an OriginateMessage:

<?php
$originate = new OriginateAction('SIP/101@outbound');
$originate->setActionID('AFBCE1436'); // The newly added function
$originate->setVariable('unique-job-id', 'AFBCE1436');
$originate->setTimeout(30000); // 30 seconds
// and so on

$pami->send($originate);
Action: Originate
Channel: SIP/101@outbound
Context: default
Exten: 8135551212
Priority: 1
Callerid: 3125551212
Timeout: 30000
Variable: unique-job-id=AFBCE1436
ActionID: AFBCE1436

Now, calling user 101@outbound fails with a timout on the Originate, giving the following two events. As you can see, the HangupEvent just says that everything is OK (Cause 16 - Normal call clearing), while the OriginateReponseEvent knows that this call timed out (Reason 3).

As you also can see, the only thing in common is now the unique job id on the channel and the same one in the Action ID.

Event: Hangup
Privilege: call,all
Timestamp: 1341587531.893844
Channel: SIP/outbound-00000018
Uniqueid: 1341587529.24
CallerIDNum: 101
CallerIDName: <unknown>
Cause: 16
Cause-txt: Normal Clearing

Event: OriginateResponse
Privilege: call,all
Timestamp: 1341828502.526406
ActionID: AFBCE1436
Response: Failure
Channel: SIP/101@outbound
Context: voiceapi
Exten:
Reason: 3
Uniqueid: <null>
CallerIDNum: <unknown>
CallerIDName: <unknown>

Example code to show how to match them:

<?php
$channel_uids = array();

/* @var $newchannel NewchannelEvent */
$getVar = new GetVarAction($newchannel->getChannel(), 'unique-job-id');
$var = $pami->send($getVar);
$channel_uid = $var->getKey('unique-job-id');
$channel_uids[$newchannel->getChannel()] = $channel_uid;

/* @var $hangup HangupEvent */

/* @var $originate OriginateResponseEvent*/
$originate_uid = $originate->getActionID();

if ($originate_uid == $channel_uids[$hangup->getChannel()] && $hangup->getChannel() == $channel_id) {
    if ($newchannel->getCause() == 16 && $originate->getReason() == 3) {
        echo "Call {$hangup_uid} timed out!" . PHP_EOL;
    }
}

As you can see, because of the ActionID we can match the OriginateResponseEvent to the HangupEvent, because, during a failure, the OriginateResponseEvent returns the originally called number as the Channel.

Original situation
In the original situation, the following events would be generated. As you can see, there's no way to match them:

Event: Hangup
Privilege: call,all
Timestamp: 1341587531.893844
Channel: SIP/outbound-00000018
Uniqueid: 1341587529.24
CallerIDNum: 101
CallerIDName: <unknown>
Cause: 16
Cause-txt: Normal Clearing

Event: OriginateResponse
Privilege: call,all
Timestamp: 1341828502.526406
ActionID: 1341828499.5255
Response: Failure
Channel: SIP/101@outbound
Context: voiceapi
Exten:
Reason: 3
Uniqueid: <null>
CallerIDNum: <unknown>
CallerIDName: <unknown>
@marcelog
Owner

Hi Jacob!

I see your point... A couple of things: I would prefer to have the actionid selection logic encapsulated in the action class, instead of having another setter (once you have one, you have many).

Now, I'm not saying that I wont accept the patch just because of this, I just want to come up with a better solution before going all the way with the pragmatic approach. This seems more like an asterisk limitation, and I think of the ActionID as something that should be restricted to the PAMI layer, and not escalate further into the user app.

Your setVariable() looks like the way to go for me, so the issue here is that you cant get that variable from the OriginateResponse, but only in your agi script, right? This is what seems to be the asterisk limitation to me.

By the way.. just tried a similar scenario, but when the event Hangup arrives, the channel does not exist anymore, so did you actually manage to do a GetVar action on this already dead channel? I'm actually referring to this code:

{{{
$getVar = new GetVarAction($hangup->getChannel(), 'unique-job-id');
$var = $pami->send($getVar); // this returns a channel not found for me
$hangup_uid = $var->getKey('unique-job-id');
}}}

@jacobkiers

(Not in order of your response, to make clarifications easier)

By the way.. just tried a similar scenario, but when the event Hangup arrives, the channel does not exist anymore, so did you actually manage to do a GetVar action on this already dead channel? [...]

Oops, my bad. Updated the example. I've pretty much written the code examples by hand. Generally, I do the getVar when I receive the newChannelEvent. And yes, this still fails when the timout is too short, or the application is too slow processing the events. In this example I used the hangupEvent, to make the example code more concise. Didn't test that, though.

Your setVariable() looks like the way to go for me, so the issue here is that you cant get that variable from the OriginateResponse, but only in your agi script, right? This is what seems to be the asterisk limitation to me.

Well, yes and no. Yes, this is an Asterisk limitation. However (the "no" part), in my example, the channel does not even get to the AGI, because the callee didn't pick up in time (before the timeout). In any other case, I would have enough with the Newchannel and Hangup Events, but in this case, the Hangup Cause is incorrect from an application perspective. I'll explain why:

  1. We start calling with a timeout of 30 seconds
  2. After 31 seconds, Asterisk stops dialing (correctly), and sends a HangupEvent with cause code 16 (also technically correct from the Channel perspective)
  3. Asterisk sends an OriginateResponse with Reason 3, indicating a timeout.
  4. Based on the Hangup, the application has successfully reached the callee, but from the OriginateResponse we know that this is not the case.

Now, because there is no way to link the OriginateResponse to the Hangup by default (as described in the main text of the Pull Request), we cannot know that the callee did not pick up the phone without also knowing the ActionID in the first place.

Since in my example the part handling the results is another different from the part that does the dialing, there's no way to know whether or not a call has timed out.

I would prefer to have the actionid selection logic encapsulated in the action class [...] I just want to come up with a better solution before going all the way with the pragmatic approach. This seems more like an asterisk limitation, and I think of the ActionID as something that should be restricted to the PAMI layer, and not escalate further into the user app.

By all means. I've added it this way because that's how the rest of the code generally works. Using setters to set parameters, that is. From my perspective, the ActionID is just another parameter (and - notwithstanding - the pragmatic approach is sometimes the way to go).

Again, if you can think of a better approach, not involving direct AGI access (which does not exist before the AGI calls), I'm all ears.

@marcelog
Owner

Let me see if I can come up with something different. A couple of things about the test included in the commit:

1.- It doesn't test the "happy case" where the actionid is actually set (you only test for the error case where the actionid is > 69)

2.- In the test code, instead of doing the try-catch thing, you can just use the expectedException annotation (http://www.phpunit.de/manual/3.2/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.exceptions.examples.ExceptionTest.php). Also, the convention for tests that I chose, is to use the "can_" prefix to signal a "happy case", and the "cannot_" prefix when testing for exceptions thrown.

3.- I think you should also throw an exception if actionid is of length 0, or false, or null something like an empty string. Or maybe just let the code crash, what do you think?

@jacobkiers

I've updated the ActionMessage and the tests as requested. The Exception will now als be thrown on empty/false/null/0-character.

@marcelog
Owner

Thanks! Just one more thing to go: the fail() is not necessary when using expectedException, because the test will automatically fail is the expected exception is never thrown

@jacobkiers

You're right. I overlooked that. Updated.

@marcelog marcelog merged commit 83fe5cf into from
@marcelog
Owner

Thanks again, Jacob :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 12, 2012
  1. @jacobkiers
  2. @jacobkiers
  3. @jacobkiers

    Updated the ActionMessage to also check for an empty ActionID. Updated

    jacobkiers authored
    test accordingly and checked for all three cases (empty, ok and too
    long).
  4. @jacobkiers
  5. @jacobkiers

    Removed fail()s in tests, because they are unnecessary. The test will

    jacobkiers authored
    fail anyway when no exception is thrown.
Commits on Jul 17, 2012
Commits on Jul 18, 2012
  1. @jacobkiers
  2. @jacobkiers
  3. @jacobkiers

    Updated the ActionMessage to also check for an empty ActionID. Updated

    jacobkiers authored
    test accordingly and checked for all three cases (empty, ok and too
    long).
  4. @jacobkiers
  5. @jacobkiers

    Removed fail()s in tests, because they are unnecessary. The test will

    jacobkiers authored
    fail anyway when no exception is thrown.
  6. @jacobkiers
Something went wrong with that request. Please try again.