| Summary: | ASTERISK-09708: Queue member gets called when user hangs up during periodic announcement | ||||
| Reporter: | Dmitry Andrianov (dimas) | Labels: | |||
| Date Opened: | 2007-06-19 07:30:16 | Date Closed: | 2007-07-19 20:39:38 | ||
| Priority: | Minor | Regression? | No | ||
| Status: | Closed/Complete | Components: | Applications/app_queue | ||
| Versions: | Frequency of Occurrence | ||||
| Related Issues: |
| ||||
| Environment: | Attachments: | ( 0) 10008.patch ( 1) 10008-2.patch ( 2) 10008-3.patch | |||
| Description: | When user hangs up while listening to queue's periodic announcement, Asterisk starts ringing the member's phone for a short period of time right after hangup. To reproduce create a queue with single member and periodic-announce-frequency=10 1. Put user on a queue, user hears MOH 2. member's softphone (X-Lite 3.0) starts ringing 3. Wait about 8 seconds 4. Member clicks "hangup" on the softphone reporting UNAVAIL to asterisk. Asterisk does not start calling member immediately (I beliebe this is because default retry = 5) 5. user still hears MOH 6. Asterisk starts playing announce to the user. While user hears announce, member's phone does not ring 7. User hangs up durng the announcement 8. Members softphone immediately start ringing for very short time (about half a second) ****** ADDITIONAL INFORMATION ****** In the background_file function there is a code /* Wait for a keypress */ res = ast_waitstream(chan, AST_DIGIT_ANY); if (res < 0 || !valid_exit(qe, res)) { res = 0; when client hangs up during periodic announcement, ast_waitstream returns -1 but the next two lines change res back to zero. So this function returns zero to say_periodic_announcement which in turn returns zero to queue_exec. | ||||
| Comments: | By: Dmitry Andrianov (dimas) 2007-06-19 08:03:10 I attached a patch which fixes the problem for me but it REALLY needs to be checked by someone who understands how app_queue works. By: Jason Parker (jparker) 2007-06-21 15:22:54 I think this is the right thing to do, but it's not used "correctly" elsewhere. With this change, it's possible that when somebody hangs up, it would log the reason for leaving the queue as EXITWITHKEY. I believe that the same also applies for play_file By: Dmitry Andrianov (dimas) 2007-06-22 09:49:34 Okay, here is bigger patch. It needs review and discussion. What I'm trying to do is to simplify code by resetting positive res back to zero if invalid digit was pressed. This makes life easier especially in the main loop because simple condition (if (res)) is enough to decide stopping the loop. Important notes about my patch: 1. Previously dialing _invalid_ digit during say_position only aborted file/digit which is currently playing and Asterisk continued saying position. Now invalid digit aborts whole say_position function. To me it is even better but who knows :) 2. Some diagnostic was lost - previously asterisk was logging some info depending on what exactly moment it detected a hangup. Like: ast_verbose(VERBOSE_PREFIX_3 "User disconnected from queue %s while waiting their turn\n", args.queuename); or ast_verbose(VERBOSE_PREFIX_3 "User disconnected from queue %s when they almost made it\n", args.queuename); Now it does not (because there is basically one exit point for abandonned calls). What I still do not like is the wait_our_turn which looks like a bit reduced copy&paste from queue_exec doing basically the same with minor differences. I would modify queue_exec's main loop to work for both cases - user is the first in the line or not. What do you think? By: Dmitry Andrianov (dimas) 2007-07-10 02:52:15 Anyone? By: Mark Michelson (mmichelson) 2007-07-18 11:01:58 I've given the patch a look-over and have tested it through and through. There seems to be a problem right now, but it isn't obvious yet why it's happening. The problem occurs if periodic announcement is configured. If a caller presses a valid DTMF to go to a particular dialplan context while the periodic announcement is playing, then instead of exiting the queue and being sent to that context, the caller hears silence until the next periodic announcement gets played. The problem does not occur if an invalid DTMF is pressed. I'll be looking into this to see if I can fix it, but if you come up with a fix in the mean time, feel free to post it. Edit: Just to be clear, this ONLY happens during a periodic announcement. It doesn't happen during the position announcement, or any other announcement. That might help to narrow the problem area down, but it hasn't helped me yet in my investigations. By: Mark Michelson (mmichelson) 2007-07-18 13:14:00 As a test, I tried removing background_file from the source completely and replacing its call with play_file instead. The problem I reported above went away. I guess the question is should we try to fix background_file or just get rid of it like I have done? In my view, it seems like a useless function and can stand to be gotten rid of. Comments? By: Dmitry Andrianov (dimas) 2007-07-18 14:36:21 Good catch! I reproduced the problem you are describing. Unfortunately, although replacing background_file with play_file seems to fix the problem it actually just hides it. The real source of the problem is the fact that valid_exit should NOT be called twice. And I completely overlooked this fact. So code need more serious fixes. I will upload patch later today. By: Dmitry Andrianov (dimas) 2007-07-18 16:36:21 I created second patch which replaces previous. This patch fixes the problem and also another small issue I noticed looking at the code. putnopvut, can you please play a little with new version? And thenks for attention to this issue. By: Mark Michelson (mmichelson) 2007-07-19 10:56:13 I reran tests on this new code and looked it over and I approve of it. It will be committed shortly. Thank you very much for taking the time to make this patch! By: Digium Subversion (svnbot) 2007-07-19 11:09:10 Repository: asterisk Revision: 75969 ------------------------------------------------------------------------ r75969 | mmichelson | 2007-07-19 11:09:09 -0500 (Thu, 19 Jul 2007) | 10 lines Changes in handling return values of several functions in app_queue. This all started as a fix for issue ASTERISK-9708 but now includes all of the following changes: 1. Simplifying the code to handle positive return values from ast API calls. 2. Removing the background_file function. 3. The fix for issue ASTERISK-9708 (closes issue ASTERISK-9708, reported and patched by dimas) ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-07-19 11:12:49 Repository: asterisk Revision: 75977 ------------------------------------------------------------------------ r75977 | mmichelson | 2007-07-19 11:12:49 -0500 (Thu, 19 Jul 2007) | 18 lines Merged revisions 75969 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r75969 | mmichelson | 2007-07-19 11:26:10 -0500 (Thu, 19 Jul 2007) | 10 lines Changes in handling return values of several functions in app_queue. This all started as a fix for issue ASTERISK-9708 but now includes all of the following changes: 1. Simplifying the code to handle positive return values from ast API calls. 2. Removing the background_file function. 3. The fix for issue ASTERISK-9708 (closes issue ASTERISK-9708, reported and patched by dimas) ........ ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-07-19 20:39:38 Repository: asterisk Revision: 76016 ------------------------------------------------------------------------ r76016 | tilghman | 2007-07-19 20:39:36 -0500 (Thu, 19 Jul 2007) | 60 lines Merged revisions 75977,75979,75981-75983 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r75977 | mmichelson | 2007-07-19 11:29:51 -0500 (Thu, 19 Jul 2007) | 18 lines Merged revisions 75969 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r75969 | mmichelson | 2007-07-19 11:26:10 -0500 (Thu, 19 Jul 2007) | 10 lines Changes in handling return values of several functions in app_queue. This all started as a fix for issue ASTERISK-9708 but now includes all of the following changes: 1. Simplifying the code to handle positive return values from ast API calls. 2. Removing the background_file function. 3. The fix for issue ASTERISK-9708 (closes issue ASTERISK-9708, reported and patched by dimas) ........ ................ r75979 | mmichelson | 2007-07-19 14:02:38 -0500 (Thu, 19 Jul 2007) | 11 lines Merged revisions 75978 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r75978 | mmichelson | 2007-07-19 13:59:30 -0500 (Thu, 19 Jul 2007) | 3 lines The diff on this looks pretty big but all I did was remove a pointless if statement (always evaluates true). ........ ................ r75981 | qwell | 2007-07-19 15:36:55 -0500 (Thu, 19 Jul 2007) | 9 lines Blocked revisions 75980 via svnmerge ........ r75980 | qwell | 2007-07-19 15:36:06 -0500 (Thu, 19 Jul 2007) | 2 lines Remove some duplicate code. ........ ................ r75982 | murf | 2007-07-19 17:00:59 -0500 (Thu, 19 Jul 2007) | 1 line This repairs a 'warning: ISO C90 forbids mixed declarations and code' message that cripples my dev-mode enabled build ................ r75983 | murf | 2007-07-19 18:24:27 -0500 (Thu, 19 Jul 2007) | 1 line After some study, thought, comparing, etc. I've backed out the previous universal mod to make ast_flags a 64 bit thing. Instead, I added a 64-bit version of ast_flags (ast_flags64), and 64-bit versions of the test-flag, set-flag, etc. macros, and an app_parse_options64 routine, and I use these in app_dial alone, to eliminate the 30-option limit it had grown to meet. There is room now for 32 more options and flags. I was heavily tempted to implement some of the other ideas that were presented, but this solution does not intro any new versions of dial, doesn't have a different API, has a minimal/zero impact on code outside of dial, and doesn't seriously (I hope) affect the code structure of dial. It's the best I can think of right now. My goal was NOT to rewrite dial. I leave that to a future, coordinated effort. ................ ------------------------------------------------------------------------ | ||||