This change has been marked as submitted.

Committed in revision 433861

Submitter:	Ashley Sanders
Branch:	branches/13
Bugs:	ASTERISK-24802
Depends On:
Blocks:

Reviewers
Groups:	asterisk-dev
People:
Repository:	Asterisk

Description:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS.

The possible values for STASISSTATUS are:
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)

And, here it is! https://reviewboard.asterisk.org/r/4520

Testing Done:

The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/

The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable:
1) The 'Babs' scenario: tests the situation where a channel is originated under normal conditions and then the channel is hungup. For this case, the test verifies that Stasis 
   correctly assigns SUCCESS to STASISSTATUS.
2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer 
   registered when call B is originated. Determines if the 'FAILED' state is correctly applied.

Issue Summary:

Description	From	Last Updated	Status
You'll want to document this new channel variable in the XML documentation at the top of the file. Something like: ...	Matt Jordan	March 25, 2015, 3:52 p.m.	Resolved
Since we get a StasisStart event, and the dialplan can't access the variable until the channel is released, I'm not ...	Matt Jordan	March 27, 2015, 9:44 a.m.	Resolved
Here, I refer to the variable as STASISSTATUS, but below, I refer to it as STASIS_STATUS. This may be a ...	Ashley Sanders	March 25, 2015, 3:53 p.m.	Resolved
Using a parameter value before asserting that it is non-NULL is just wrong. That's just closing the barn door after ...	rmudgett	March 25, 2015, 3:49 p.m.	Resolved
Idem.	rmudgett	March 25, 2015, 3:49 p.m.	Dropped
Nitpick: "A failure occurred when executing the Stasis application."	Joshua Colp	March 31, 2015, 4:54 p.m.	Resolved

Description

Changed from:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASIS_STATUS.

The possible values for STASIS_STATUS are:
INITIALIZING    - Indicates Stasis is starting
ACTIVE          - The channel is active in Stasis
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)

Changed to:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASIS_STATUS.

The possible values for STASIS_STATUS are:
INITIALIZING    - Indicates Stasis is starting
ACTIVE          - The channel is active in Stasis
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)
https://reviewboard.asterisk.org/r/4520

Description:

Added a link to the testsuite review.

Make sure you document in the CHANGES file that app_stasis now sets a channel variable upon application completion.

./branches/13/apps/app_stasis.c (Diff revision 1)

You'll want to document this new channel variable in the XML documentation at the top of the file. Something like:

			<para>This application will set the following channel variable upon completion:</para>
			<variablelist>
				<variable name="STASISSTATUS">
					<para>This indicates the status of the execution of the Stasis application.</para>
					<value name="SUCCESS" />
					<value name="FAILED" />
				</variable>
			</variablelist>

Show all issues

Joshua Colp March 24, 2015, 8:31 a.m. (March 24, 2015, 8:31 a.m.)

You also need to clarify (like you did in the description) what success and failed mean. If I'm a user looking at this variable I'd like to know more about what "failed" means and when it can happen.

./branches/13/res/res_stasis.c (Diff revision 1)

Since we get a StasisStart event, and the dialplan can't access the variable until the channel is released, I'm not sure we really need to set the STASIS_START variables to INITIALIZING and ACTIVE.

With respect to the INITIALIZING value, a Stasis application won't see that value being set on the channel unless they've already subscribed to the channel. The implicit subscription is created as a result of the control_create call, which happens after the variable is set.

With respect to the ACTIVE value, this is only set after the StasisStart message is sent - at which point, the application is aware of the channel.

When the channel returns from the application, the code in app_stasis will set the value to either "SUCCESS" or "FAILED", which means these values will only have meaning for clients watching the state of the channel through an API.

There may be some value in having AMI clients know this state, but I think it may be rather minor - and this will generate at least two Stasis messages per channel entering into the Stasis dialplan application - so I'm not sure it is really worth the overhead.

Show all issues

./branches/13/apps/app_stasis.c (Diff revision 1)

Here, I refer to the variable as STASISSTATUS, but below, I refer to it as STASIS_STATUS.

This may be a moot point wrt to the source after fixing the issue opened by Matt Jordan, but will impact the test.

Show all issues

./branches/13/res/res_stasis.c (Diff revision 1)

+1 to this. I think following the pattern for other applications that do this is good - they just do success/failed.

Just a comment on the code review itself: Please always fill in the "Testing Done" field with the testing you've done.

Ashley Sanders March 25, 2015, 3:56 p.m. (March 25, 2015, 3:56 p.m.)

Since I added to the description the link to the testsuite change (which describes the testing done), I presumed the 'Testing Done' field at that point was redundant. However, perhaps redundancy is not a bad thing with respect to documentation. I will add the link and a brief blurb to that field also, for clarity.

./branches/13/apps/app_stasis.c (Diff revision 1)

Using a parameter value before asserting that it is non-NULL is just wrong.

That's just closing the barn door after the barn has been destroyed by the tornado. :)

Show all issues

Ashley Sanders March 27, 2015, 9:54 a.m. (March 27, 2015, 9:54 a.m.)
```
Got it.
```

./branches/13/res/res_stasis.c (Diff revision 1)

Idem.

Show all issues

Ashley Sanders March 27, 2015, 9:54 a.m. (March 27, 2015, 9:54 a.m.)

I am dropping this issue since the referenced line is no longer existent after applying the changes brought up by Matt Jordan.

Testing Done

Changed from:

Changed to:

The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/

The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable:
1) The 'Babs' scenario: tests a nominal path of Stasis to verify the 'ACTIVE' state is correctly applied. For this test, a call is originated under normal conditions and then the system is polled for the value of STASIS_STATUS before the channel is hung up.
2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied.

Description

Changed from:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASIS_STATUS.

The possible values for STASIS_STATUS are:
INITIALIZING    - Indicates Stasis is starting
ACTIVE          - The channel is active in Stasis
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)
https://reviewboard.asterisk.org/r/4520

Changed to:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS.

The possible values for STASIS_STATUS are:
INITIALIZING    - Indicates Stasis is starting
ACTIVE          - The channel is active in Stasis
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)
https://reviewboard.asterisk.org/r/4520

Description:

Updated the description and testing done fields to depict:
A) The name of the channel variable is: STASISSTATUS, not STASIS_STATUS
B) The testing done field now contains a link and the blurb from the Testsuite test, for clarity.

Diff
- added Diff r2 - Show changes

Description:

Addresses review feedback from mjordan, jcolp, and rmudgett.

Description

Changed from:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS.

The possible values for STASIS_STATUS are:
INITIALIZING    - Indicates Stasis is starting
ACTIVE          - The channel is active in Stasis
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)
https://reviewboard.asterisk.org/r/4520

Changed to:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS.

The possible values for STASIS_STATUS are:
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)

And, here it is! https://reviewboard.asterisk.org/r/4520

Description:

Updated the description to depict the revised values of STASISSTATUS.

Testing Done

Changed from:

The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/

The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable:
1) The 'Babs' scenario: tests a nominal path of Stasis to verify the 'ACTIVE' state is correctly applied. For this test, a call is originated under normal conditions and then the system is polled for the value of STASIS_STATUS before the channel is hung up.
2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied.

Changed to:

The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/

The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable:
1) The 'Babs' scenario: tests a nominal path of Stasis to verify the 'ACTIVE' state is correctly applied. For this test, a call is originated under normal conditions and then the system is polled for the value of STASISSTATUS before the channel is hung up.
2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied.

Description

Changed from:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS.

The possible values for STASIS_STATUS are:
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)

And, here it is! https://reviewboard.asterisk.org/r/4520

Changed to:

When an error occurs while writing to a web socket, the web socket is disconnected and the event is logged. A side-effect of this, however, is that any application on the other side waiting for a response from Stasis is left hanging indefinitely (as there is no mechanism presently available for notifying interested parties about web socket error states in Stasis).

To remedy this scenario, this patch introduces a new channel variable: STASISSTATUS.

The possible values for STASISSTATUS are:
SUCCESS         - The channel has exited Stasis without any failures
FAILED          - Something caused Stasis to croak. Some (not all) possible reasons for this: 
                    - The app registry is not instantiated; 
                    - The app requested is not registered; 
                    - The app requested is not active; 
                    - Stasis couldn't send a start message

 ***Note*** This is just the patch to the Asterisk source. The testsuite review is coming soon to a reviewboard near you (well, this reviewboard.)

And, here it is! https://reviewboard.asterisk.org/r/4520

Description:

Changed the references to STASIS_STATUS to STASISSTATUS.

Testing Done

Changed from:

The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/

The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable:
1) The 'Babs' scenario: tests a nominal path of Stasis to verify the 'ACTIVE' state is correctly applied. For this test, a call is originated under normal conditions and then the system is polled for the value of STASISSTATUS before the channel is hung up.
2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied.

Changed to:

The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/

The test in Testsuite exercises two scenarios to elicit updates to the STASISSTATUS channel variable:
1) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
2) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied.

Description:

Updated the 'Testing Done' field to reflect the modifications to the testsuite issue's description change.

Testing Done

Changed from:

The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/

The test in Testsuite exercises two scenarios to elicit updates to the STASISSTATUS channel variable:
1) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
2) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer registered when call B is originated. Determines if the 'FAILED' state is correctly applied.

Changed to:

The testing done for this issue can be found at: https://reviewboard.asterisk.org/r/4520/

The test in Testsuite exercises three scenarios to elicit updates to the STASISSTATUS channel variable:
1) The 'Babs' scenario: tests the situation where a channel is originated under normal conditions and then the channel is hungup. For this case, the test verifies that Stasis 
   correctly assigns SUCCESS to STASISSTATUS.
2) The 'Bugs' scenario: tests the situation where a call is originated requesting an app that was never registered in Stasis to verify the 'FAILED' state is correctly applied.
3) The 'Buster' scenario: tests the situation where an app that was registered in Stasis when call A was originated (and while call A is still active) but is no longer 
   registered when call B is originated. Determines if the 'FAILED' state is correctly applied.

Description:

Updated the 'testing done' field to depict the re-added 'Babs' scenario.

./branches/13/apps/app_stasis.c (Diff revision 2)

Nitpick: "A failure occurred when executing the Stasis application."

Show all issues

Otherwise good.

Diff
- added Diff r3 - Show changes

Description:

Fixed the docstring according to Joshua Colp's suggestion.

status changed from pending to submitted

Description:

Committed in revision 433861

You have a pending review.

Other reviews

Your comment

Review Board 1.7.16

This change has been marked as submitted.

Asterisk: stasis: set a channel variable on websocket disconnect error