[Home]

Summary:ASTERISK-05781: [patch] Cannot AddQueueMember on realtime queue, if queue not yet loaded
Reporter:Jason Parker (jparker)Labels:
Date Opened:2005-12-05 11:15:54.000-0600Date Closed:2006-01-21 13:32:49.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_queue
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20060113__bug5936.diff.txt
( 1) app_queue-8103.diff
( 2) app_queue-r7353-20051206-2035.diff
Description:If you try to AddQueueMember to a realtime queue before calling Queue(), you'll get the following message.

"Dec  5 10:02:02 WARNING[10297]: app_queue.c:2824 aqm_exec: Unable to add interface to queue 'blah': No such queue"

If you have no static (realtime) queue members defined, it would be impossible for the first caller after a reload to be able to join a queue, since it will be empty.

****** ADDITIONAL INFORMATION ******

I'll see if I can split out the adding of the queue to the list (in join_queue() currently) to another function, and call that function from any of the other things, like "show queue blah", AddQueueMember, etc.

This is somewhat related to ASTERISK-5767, since they both deal with realtime queues and dynamic members.

As always, disclaimer is on file.
Comments:By: Jason Parker (jparker) 2005-12-05 12:01:13.000-0600

Patch uploaded.

This fixes "show queue blah" (which I believe also fixes parts of manager), as well as AddQueueMember.  Is there anything obvious I missed?  Any potential breakage, or deadlocks?

Disclaimer on file.
By: Jason Parker (jparker) 2005-12-05 12:02:47.000-0600

crap, qlock...  hold on
By: Jason Parker (jparker) 2005-12-05 12:13:10.000-0600

Better.
By: Tilghman Lesher (tilghman) 2005-12-05 13:07:35.000-0600

You should reverse the order of the search.  That is, you should search memory FIRST for the queue, and if not found, you should only then look into realtime.  Memory searches are a lot faster than realtime; in addition, a realtime queue should be able to be cached in memory for better performance.
By: Jason Parker (jparker) 2005-12-05 14:23:07.000-0600

reload_queue_rt already checks the "in-core list" first.  Is this sufficient?

       /* Find the queue in the in-core list (we will create a new one if not found). */
       q = queues;
       prev_q = NULL;
       while (q) {
               if (!strcasecmp(q->name, queuename)) {
                       break;
               }
               q = q->next;
               prev_q = q;
       }

       /* Static queues override realtime. */
       if (q) {
               ast_mutex_lock(&q->lock);
               if (!q->realtime) {
                       if (q->dead) {
                               ast_mutex_unlock(&q->lock);
                               return NULL;
                       } else {
                               return q;
                       }
               }
       }
By: Tilghman Lesher (tilghman) 2005-12-05 15:32:56.000-0600

Okay, this code is WAY too messy.  First of all, any function that locks the global qlock must also unlock the global qlock.  Period.  No holding of that lock past the return of the function that locks it.  (You can, however, call other functions with it locked.)

Second, the reason why I want you to check in memory FIRST is that database lookups are expensive.  If the structure is already in memory, don't call any realtime function to reload the structure.  It's there in memory; use it.  If you need to separate part of the core of add_to_queue into a separate function so it can be called first on each of the in-memory queues, then on a queue loaded from realtime (but ONLY if an in-memory queue is not found), that's fine; in fact, I think this is the way it should be done.

Third, "reload" in a function name should only be used when you're actually doing a reload on the module.  Your reload_queue_rt should be more aptly named as a find function (such as find_rt_queue_by_name).  This increases code clarity.

Similarly, you should fix join_queue and __queues_show such that you do not call a realtime function in all cases, but only in cases where there is not a
matching in-memory queue.
By: Jason Parker (jparker) 2005-12-05 16:46:32.000-0600

1) I'll see what I can do about that.  What about the q->lock that reload_queue_rt sets?

2) As far as I can see, I didn't change that functionality, and reload_queue_rt already does (as I pointed out previously) check the in memory structure first.

3) I didn't write reload_queue_rt, and I don't think it's in the scope of this bug to rename it.

edit: I see you committed a patch for 3) - could you please note things like this in the future, so I don't get conflicts when I svn up?  Only way I found out, was by being subscribed to svn-commits.  I wouldn't have known otherwise.


By: Tilghman Lesher (tilghman) 2005-12-06 14:56:30.000-0600

Holding q->lock past the return is fine.  A number of find_* functions in Asterisk do exactly this.

Let me see a new patch, and I'll review the code again.
By: Jason Parker (jparker) 2005-12-06 20:39:34.000-0600

How's that?
By: Tilghman Lesher (tilghman) 2005-12-06 20:43:03.000-0600

load_realtime_queue() does a query to the database before checking in-memory queues.
By: Jason Parker (jparker) 2005-12-06 21:35:56.000-0600

Delete 2030 patch - had some extra code in it.
By: Jason Parker (jparker) 2006-01-06 01:11:53.000-0600

FYI, this is waiting for another review.  I've been using it for about a month now, with no problems.
By: Tilghman Lesher (tilghman) 2006-01-13 16:19:54.000-0600

I made a few modifications to your patch, making it a bit less redundant.  Please test.
By: Jason Parker (jparker) 2006-01-13 16:22:48.000-0600

Will test Monday
By: Jason Parker (jparker) 2006-01-16 11:45:34.000-0600

Your patch didn't apply.  This one does.

It seems to work alright.
By: Tilghman Lesher (tilghman) 2006-01-16 12:05:48.000-0600

Are you patching against 1.2 or against trunk?  This should go into 1.2, as it is a legitimate bug.
By: Jason Parker (jparker) 2006-01-16 17:31:22.000-0600

I use trunk, sorry.

I looked at 1.2, and I agree, your patch will work fine there.  The patch I uploaded is for trunk.

All that was changed between your patch and my latest patch, is the renamed function.  You've got my vote for a commit on this one.
By: Tilghman Lesher (tilghman) 2006-01-21 12:16:49.000-0600

Okay, committed to 1.2.  However, I can't merge it, as app_queue in trunk is way too different.
By: Jason Parker (jparker) 2006-01-21 12:46:28.000-0600

My app-queue-8103.diff should work on trunk.  It's basically the same as yours, except the modified function names.
By: Tilghman Lesher (tilghman) 2006-01-21 13:32:48.000-0600

Committed to trunk