/ Zope / Apsis / Pound Mailing List / Archive / 2011 / 2011-03 / RE: Queue implementation errors?

[ << ] [ >> ]

[ redirect to https:// with same hostname / Andrzej ... ] [ Pound for redundancy, letting sessions dry out / ... ]

RE: Queue implementation errors?
Steven van der Vegt <steven(at)echelon.nl>
2011-03-03 18:08:37 [ SNIP ]
Hi Joe,

Sorry for the delays. Here is the diff.
The replacement of the signal is to prevent false wakeups. It should be
correct, but I can throw it through a modelchecker if you like?

Kind regards,

Steven van der vegt

-----Oorspronkelijk bericht-----
Van: Joe Gooch [mailto:mrwizard(at)k12system.com] 
Verzonden: vrijdag 25 februari 2011 18:25
Aan: pound(at)apsis.ch
Onderwerp: [Pound Mailing List] RE: Queue implementation errors?

I looked at it and I agree, the crux of the problem seems to be the "next"
variable in the tree isn't always populated.

You moved the signal too... Not sure if that's right or not.

Could provide your modified .c file so we could make a proper diff?

Thanks.

Joe

> -----Original Message-----
> From: Steven van der Vegt [mailto:steven(at)echelon.nl]
> Sent: Friday, February 25, 2011 11:38 AM
> To: pound(at)apsis.ch
> Subject: [Pound Mailing List] RE: Queue implementation errors?
> 
> Dear List,
> 
> Can anyone comment on my proposed changes?
> 
> Thanks! :)
> 
> Steven
> 
> -----Oorspronkelijk bericht-----
> Van: Steven van der Vegt [mailto:steven(at)echelon.nl]
> Verzonden: woensdag 16 februari 2011 17:40
> Aan: pound(at)apsis.ch
> Onderwerp: [Pound Mailing List] Queue implementation errors?
> 
> Hi Folks,
> 
> I wanted to test the 2.6c version on a testsystem of ours but I ran
> into trouble. Every time 1 request was handled I got this error:
> 
> MONITOR: worker exited on signal 11, restarting...
> 
> After some research I found out it was the memcopy in do_http() which
> causes the error:
> 
> from_host =arg *)arg)->from_host;
> memcpy(&from_host_addr, from_host.ai_addr, from_host.ai_addrlen);
> from_host.ai_addr =t sockaddr *)&from_host_addr;
> 
> The value from_host.ai_addrlen was equal to 5394258 and after some
> debugging it seems the whole struct contained bogus.
> 
> Ok, let's find out why?
> 
> The do_http() function is called from thr_http() which has some strange
> while loop in it. It fetches work from a queue with the call
> get_thr_arg() but this call is allowed to return a NULL value
> sometimes. If this happens than ask again...
> Well, this wasn't a NULL value so there must be something else going
> on. Maybe the queue?
> 
> The code contained some errors (as far as my concurrent and queue
> knowledge go's). I reimplemented them to the following code. It solved
> my errors and I think the strange while-loop in the thr_http function
> is not needed anymore.
> 
> Can you please comment on my changes?
> 
> int
> put_thr_arg(thr_arg *arg)
> {
>     thr_arg *res;
> 
>     if((res =(sizeof(thr_arg))) = {
>         logmsg(LOG_WARNING, "thr_arg malloc");
>         return -1;
>     }
>     memcpy(res, arg, sizeof(thr_arg));
>     (void)pthread_mutex_lock(&arg_mut);
> 
>     res->next =
> 
>     if (last = {
>         first =
>     } else {
>         last->next >         last >     }
> 
>     pthread_cond_broadcast(&arg_cond);
>     (void)pthread_mutex_unlock(&arg_mut);
>     return 0;
> }
> 
> /*
>  * get a request from the queue
>  */
> thr_arg *
> get_thr_arg(void)
> {
>     thr_arg *res =
> 
>     (void)pthread_mutex_lock(&arg_mut);
>     while (first =
>         (void)pthread_cond_wait(&arg_cond, &arg_mut);
> 
>     res =
>     if((first =ext) =
>         last =
>     else
>         pthread_cond_signal(&arg_cond);
> 
>     (void)pthread_mutex_unlock(&arg_mut);
> 
>     return res;
> }
> 
> Thanks!
> 
> Kind regards,
> 
> Steven van der Vegt
> 
> --
> To unsubscribe send an email with subject unsubscribe to
> pound(at)apsis.ch.
> Please contact roseg(at)apsis.ch for questions.
> 
> --
> To unsubscribe send an email with subject unsubscribe to
> pound(at)apsis.ch.
> Please contact roseg(at)apsis.ch for questions.

--
To unsubscribe send an email with subject unsubscribe to pound(at)apsis.ch.
Please contact roseg(at)apsis.ch for questions.

Attachments:  
pound_queue_http.c.diff application/octet-stream 152 Bytes
pound_queue_pound.c.diff application/octet-stream 841 Bytes

Re: [Pound Mailing List] Queue implementation errors?
Robert Segall <roseg(at)apsis.ch>
2011-03-07 09:38:00 [ SNIP ]
On Wed, 2011-02-16 at 16:40 +0000, Steven van der Vegt wrote:
> Hi Folks,
> 
> I wanted to test the 2.6c version on a testsystem of ours but I ran into
trouble. Every time 1 request was handled I got this error:
> 
> MONITOR: worker exited on signal 11, restarting...
> 
> After some research I found out it was the memcopy in do_http() which causes
the error:
> 
> from_host = ((thr_arg *)arg)->from_host;
> memcpy(&from_host_addr, from_host.ai_addr, from_host.ai_addrlen);
> from_host.ai_addr = (struct sockaddr *)&from_host_addr;
> 
> The value from_host.ai_addrlen was equal to 5394258 and after some debugging
it seems the whole struct contained bogus. 
> 
> Ok, let's find out why?
> 
> The do_http() function is called from thr_http() which has some strange while
loop in it. It fetches work from a queue with the call get_thr_arg() but this
call is allowed to return a NULL value sometimes. If this happens than ask
again...
> Well, this wasn't a NULL value so there must be something else going on.
Maybe the queue?
> 
> The code contained some errors (as far as my concurrent and queue knowledge
go's). I reimplemented them to the following code. It solved my errors and I
think the strange while-loop in the thr_http function is not needed anymore.
> 
> Can you please comment on my changes?
> 
> int
> put_thr_arg(thr_arg *arg)
> {
>     thr_arg *res;
> 
>     if((res = malloc(sizeof(thr_arg))) == NULL) {
>         logmsg(LOG_WARNING, "thr_arg malloc");
>         return -1;
>     }
>     memcpy(res, arg, sizeof(thr_arg));
>     (void)pthread_mutex_lock(&arg_mut);
> 
>     res->next = NULL;
> 
>     if (last == NULL) {
>         first = last = res;
>     } else {
>         last->next = res;
>         last = res;
>     }
> 
>     pthread_cond_broadcast(&arg_cond);
>     (void)pthread_mutex_unlock(&arg_mut);
>     return 0;
> }
> 
> /*
>  * get a request from the queue
>  */
> thr_arg *
> get_thr_arg(void)
> {
>     thr_arg *res = NULL;
> 
>     (void)pthread_mutex_lock(&arg_mut);
>     while (first == NULL)
>         (void)pthread_cond_wait(&arg_cond, &arg_mut);
> 
>     res = first;
>     if((first = res->next) == NULL)
>         last = NULL;
>     else
>         pthread_cond_signal(&arg_cond);
> 
>     (void)pthread_mutex_unlock(&arg_mut);
> 
>     return res;
> }
> 
> Thanks!
> 
> Kind regards,
> 
> Steven van der Vegt
> 
> --
> To unsubscribe send an email with subject unsubscribe to pound(at)apsis.ch.
> Please contact roseg(at)apsis.ch for questions.

>From what I see, you have made two changes:

1. you have moved the NULL check from the caller in http.c to the getter
in pound.c (a NULL check is required in any case, as the condition
wake-up may occur even when there is no data to be consumed)

2. you have replaced the cond_signal with a cond_broadcast in the
enqueing function

The first change should make no difference (it is just moving the actual
check to another place). The second will cause more threads to wake-up,
quite often more than are required. I must admit am not at all clear how
should this help.
-- 
Robert Segall
Apsis GmbH
Postfach, Uetikon am See, CH-8707
Tel: +41-32-512 30 19


RE: [Pound Mailing List] Queue implementation errors?
Joe Gooch <mrwizard(at)k12system.com>
2011-03-07 15:48:29 [ SNIP ]
As I see it this is the most important change:
>     (void)pthread_mutex_lock(&arg_mut);
> 
>     res->next = NULL;
> 
>     if (last == NULL) {


res->next isn't set to null in the 2.6c code.

Joe

> -----Original Message-----
> From: Robert Segall [mailto:roseg(at)apsis.ch]
> Sent: Monday, March 07, 2011 3:38 AM
> To: pound(at)apsis.ch
> Subject: Re: [Pound Mailing List] Queue implementation errors?
> 
> On Wed, 2011-02-16 at 16:40 +0000, Steven van der Vegt wrote:
> > Hi Folks,
> >
> > I wanted to test the 2.6c version on a testsystem of ours but I ran
> into trouble. Every time 1 request was handled I got this error:
> >
> > MONITOR: worker exited on signal 11, restarting...
> >
> > After some research I found out it was the memcopy in do_http() which
> causes the error:
> >
> > from_host = ((thr_arg *)arg)->from_host;
> > memcpy(&from_host_addr, from_host.ai_addr, from_host.ai_addrlen);
> > from_host.ai_addr = (struct sockaddr *)&from_host_addr;
> >
> > The value from_host.ai_addrlen was equal to 5394258 and after some
> debugging it seems the whole struct contained bogus.
> >
> > Ok, let's find out why?
> >
> > The do_http() function is called from thr_http() which has some
> strange while loop in it. It fetches work from a queue with the call
> get_thr_arg() but this call is allowed to return a NULL value
> sometimes. If this happens than ask again...
> > Well, this wasn't a NULL value so there must be something else going
> on. Maybe the queue?
> >
> > The code contained some errors (as far as my concurrent and queue
> knowledge go's). I reimplemented them to the following code. It solved
> my errors and I think the strange while-loop in the thr_http function
> is not needed anymore.
> >
> > Can you please comment on my changes?
> >
> > int
> > put_thr_arg(thr_arg *arg)
> > {
> >     thr_arg *res;
> >
> >     if((res = malloc(sizeof(thr_arg))) == NULL) {
> >         logmsg(LOG_WARNING, "thr_arg malloc");
> >         return -1;
> >     }
> >     memcpy(res, arg, sizeof(thr_arg));
> >     (void)pthread_mutex_lock(&arg_mut);
> >
> >     res->next = NULL;
> >
> >     if (last == NULL) {
> >         first = last = res;
> >     } else {
> >         last->next = res;
> >         last = res;
> >     }
> >
> >     pthread_cond_broadcast(&arg_cond);
> >     (void)pthread_mutex_unlock(&arg_mut);
> >     return 0;
> > }
> >
> > /*
> >  * get a request from the queue
> >  */
> > thr_arg *
> > get_thr_arg(void)
> > {
> >     thr_arg *res = NULL;
> >
> >     (void)pthread_mutex_lock(&arg_mut);
> >     while (first == NULL)
> >         (void)pthread_cond_wait(&arg_cond, &arg_mut);
> >
> >     res = first;
> >     if((first = res->next) == NULL)
> >         last = NULL;
> >     else
> >         pthread_cond_signal(&arg_cond);
> >
> >     (void)pthread_mutex_unlock(&arg_mut);
> >
> >     return res;
> > }
> >
> > Thanks!
> >
> > Kind regards,
> >
> > Steven van der Vegt
> >
> > --
> > To unsubscribe send an email with subject unsubscribe to
> pound(at)apsis.ch.
> > Please contact roseg(at)apsis.ch for questions.
> 
> >From what I see, you have made two changes:
> 
> 1. you have moved the NULL check from the caller in http.c to the
> getter
> in pound.c (a NULL check is required in any case, as the condition
> wake-up may occur even when there is no data to be consumed)
> 
> 2. you have replaced the cond_signal with a cond_broadcast in the
> enqueing function
> 
> The first change should make no difference (it is just moving the
> actual
> check to another place). The second will cause more threads to wake-up,
> quite often more than are required. I must admit am not at all clear
> how
> should this help.
> --
> Robert Segall
> Apsis GmbH
> Postfach, Uetikon am See, CH-8707
> Tel: +41-32-512 30 19
> 
> 
> --
> To unsubscribe send an email with subject unsubscribe to
> pound(at)apsis.ch.
> Please contact roseg(at)apsis.ch for questions.

RE: [Pound Mailing List] Queue implementation errors?
Joe Gooch <mrwizard(at)k12system.com>
2011-03-07 15:57:26 [ SNIP ]
As I see it this is the most important change:
>     (void)pthread_mutex_lock(&arg_mut);
> 
>     res->next = NULL;
> 
>     if (last == NULL) {


res->next isn't set to null in the 2.6c code.

Joe

> -----Original Message-----
> From: Robert Segall [mailto:roseg(at)apsis.ch]
> Sent: Monday, March 07, 2011 3:38 AM
> To: pound(at)apsis.ch
> Subject: Re: [Pound Mailing List] Queue implementation errors?
> 
> On Wed, 2011-02-16 at 16:40 +0000, Steven van der Vegt wrote:
> > Hi Folks,
> >
> > I wanted to test the 2.6c version on a testsystem of ours but I ran
> into trouble. Every time 1 request was handled I got this error:
> >
> > MONITOR: worker exited on signal 11, restarting...
> >
> > After some research I found out it was the memcopy in do_http() which
> causes the error:
> >
> > from_host = ((thr_arg *)arg)->from_host;
> > memcpy(&from_host_addr, from_host.ai_addr, from_host.ai_addrlen);
> > from_host.ai_addr = (struct sockaddr *)&from_host_addr;
> >
> > The value from_host.ai_addrlen was equal to 5394258 and after some
> debugging it seems the whole struct contained bogus.
> >
> > Ok, let's find out why?
> >
> > The do_http() function is called from thr_http() which has some
> strange while loop in it. It fetches work from a queue with the call
> get_thr_arg() but this call is allowed to return a NULL value
> sometimes. If this happens than ask again...
> > Well, this wasn't a NULL value so there must be something else going
> on. Maybe the queue?
> >
> > The code contained some errors (as far as my concurrent and queue
> knowledge go's). I reimplemented them to the following code. It solved
> my errors and I think the strange while-loop in the thr_http function
> is not needed anymore.
> >
> > Can you please comment on my changes?
> >
> > int
> > put_thr_arg(thr_arg *arg)
> > {
> >     thr_arg *res;
> >
> >     if((res = malloc(sizeof(thr_arg))) == NULL) {
> >         logmsg(LOG_WARNING, "thr_arg malloc");
> >         return -1;
> >     }
> >     memcpy(res, arg, sizeof(thr_arg));
> >     (void)pthread_mutex_lock(&arg_mut);
> >
> >     res->next = NULL;
> >
> >     if (last == NULL) {
> >         first = last = res;
> >     } else {
> >         last->next = res;
> >         last = res;
> >     }
> >
> >     pthread_cond_broadcast(&arg_cond);
> >     (void)pthread_mutex_unlock(&arg_mut);
> >     return 0;
> > }
> >
> > /*
> >  * get a request from the queue
> >  */
> > thr_arg *
> > get_thr_arg(void)
> > {
> >     thr_arg *res = NULL;
> >
> >     (void)pthread_mutex_lock(&arg_mut);
> >     while (first == NULL)
> >         (void)pthread_cond_wait(&arg_cond, &arg_mut);
> >
> >     res = first;
> >     if((first = res->next) == NULL)
> >         last = NULL;
> >     else
> >         pthread_cond_signal(&arg_cond);
> >
> >     (void)pthread_mutex_unlock(&arg_mut);
> >
> >     return res;
> > }
> >
> > Thanks!
> >
> > Kind regards,
> >
> > Steven van der Vegt
> >
> > --
> > To unsubscribe send an email with subject unsubscribe to
> pound(at)apsis.ch.
> > Please contact roseg(at)apsis.ch for questions.
> 
> >From what I see, you have made two changes:
> 
> 1. you have moved the NULL check from the caller in http.c to the
> getter
> in pound.c (a NULL check is required in any case, as the condition
> wake-up may occur even when there is no data to be consumed)
> 
> 2. you have replaced the cond_signal with a cond_broadcast in the
> enqueing function
> 
> The first change should make no difference (it is just moving the
> actual
> check to another place). The second will cause more threads to wake-up,
> quite often more than are required. I must admit am not at all clear
> how
> should this help.
> --
> ?Robert Segall
> Apsis GmbH
> Postfach, Uetikon am See, CH-8707
> Tel: +41-32-512 30 19
> 
> 
> --
> To unsubscribe send an email with subject unsubscribe to
> pound(at)apsis.ch.
> Please contact roseg(at)apsis.ch for questions.

RE: [Pound Mailing List] Queue implementation errors?
Steven van der Vegt <steven(at)echelon.nl>
2011-03-09 11:54:22 [ SNIP ]
> From what I see, you have made two changes:

> 1. you have moved the NULL check from the caller in http.c to the getter
> in pound.c (a NULL check is required in any case, as the condition
> wake-up may occur even when there is no data to be consumed)

This is true, but why place this responsibility by the caller? This get_thr_arg
is meant to be partial right? Now it is not. Also I think the code is more
clear now, but that's of course a matter of taste :)

> 2. you have replaced the cond_signal with a cond_broadcast in the
> enqueing function

> The first change should make no difference (it is just moving the actual
> check to another place). The second will cause more threads to wake-up,
> quite often more than are required. I must admit am not at all clear how
> should this help.

This will prevent lost-wakeups. So instead of waking-up one thread, You wake
them all up.

I also made an error in my code. After every dequeue() I signaled This is not
necessary. I attached my new pound.c file.

Steven

-----Oorspronkelijk bericht-----
Van: Robert Segall [mailto:roseg(at)apsis.ch] 
Verzonden: maandag 7 maart 2011 9:38
Aan: pound(at)apsis.ch
Onderwerp: Re: [Pound Mailing List] Queue implementation errors?

On Wed, 2011-02-16 at 16:40 +0000, Steven van der Vegt wrote:
> Hi Folks,
> 
> I wanted to test the 2.6c version on a testsystem of ours but I ran into
trouble. Every time 1 request was handled I got this error:
> 
> MONITOR: worker exited on signal 11, restarting...
> 
> After some research I found out it was the memcopy in do_http() which causes
the error:
> 
> from_host = ((thr_arg *)arg)->from_host;
> memcpy(&from_host_addr, from_host.ai_addr, from_host.ai_addrlen);
> from_host.ai_addr = (struct sockaddr *)&from_host_addr;
> 
> The value from_host.ai_addrlen was equal to 5394258 and after some debugging
it seems the whole struct contained bogus. 
> 
> Ok, let's find out why?
> 
> The do_http() function is called from thr_http() which has some strange while
loop in it. It fetches work from a queue with the call get_thr_arg() but this
call is allowed to return a NULL value sometimes. If this happens than ask
again...
> Well, this wasn't a NULL value so there must be something else going on.
Maybe the queue?
> 
> The code contained some errors (as far as my concurrent and queue knowledge
go's). I reimplemented them to the following code. It solved my errors and I
think the strange while-loop in the thr_http function is not needed anymore.
> 
> Can you please comment on my changes?
> 
> int
> put_thr_arg(thr_arg *arg)
> {
>     thr_arg *res;
> 
>     if((res = malloc(sizeof(thr_arg))) == NULL) {
>         logmsg(LOG_WARNING, "thr_arg malloc");
>         return -1;
>     }
>     memcpy(res, arg, sizeof(thr_arg));
>     (void)pthread_mutex_lock(&arg_mut);
> 
>     res->next = NULL;
> 
>     if (last == NULL) {
>         first = last = res;
>     } else {
>         last->next = res;
>         last = res;
>     }
> 
>     pthread_cond_broadcast(&arg_cond);
>     (void)pthread_mutex_unlock(&arg_mut);
>     return 0;
> }
> 
> /*
>  * get a request from the queue
>  */
> thr_arg *
> get_thr_arg(void)
> {
>     thr_arg *res = NULL;
> 
>     (void)pthread_mutex_lock(&arg_mut);
>     while (first == NULL)
>         (void)pthread_cond_wait(&arg_cond, &arg_mut);
> 
>     res = first;
>     if((first = res->next) == NULL)
>         last = NULL;
>     else
>         pthread_cond_signal(&arg_cond);
> 
>     (void)pthread_mutex_unlock(&arg_mut);
> 
>     return res;
> }
> 
> Thanks!
> 
> Kind regards,
> 
> Steven van der Vegt
> 
> --
> To unsubscribe send an email with subject unsubscribe to pound(at)apsis.ch.
> Please contact roseg(at)apsis.ch for questions.

>From what I see, you have made two changes:

1. you have moved the NULL check from the caller in http.c to the getter
in pound.c (a NULL check is required in any case, as the condition
wake-up may occur even when there is no data to be consumed)

2. you have replaced the cond_signal with a cond_broadcast in the
enqueing function

The first change should make no difference (it is just moving the actual
check to another place). The second will cause more threads to wake-up,
quite often more than are required. I must admit am not at all clear how
should this help.
-- 
Robert Segall
Apsis GmbH
Postfach, Uetikon am See, CH-8707
Tel: +41-32-512 30 19


--
To unsubscribe send an email with subject unsubscribe to pound(at)apsis.ch.
Please contact roseg(at)apsis.ch for questions.
Attachments:  
pound.c text/plain 17496 Bytes

RE: [Pound Mailing List] Queue implementation errors?
Robert Segall <roseg(at)apsis.ch>
2011-03-17 17:44:34 [ SNIP ]
On Wed, 2011-03-09 at 10:54 +0000, Steven van der Vegt wrote:
> > From what I see, you have made two changes:
> 
> > 1. you have moved the NULL check from the caller in http.c to the getter
> > in pound.c (a NULL check is required in any case, as the condition
> > wake-up may occur even when there is no data to be consumed)
> 
> This is true, but why place this responsibility by the caller? This
get_thr_arg is meant to be partial right? Now it is not. Also I think the code
is more clear now, but that's of course a matter of taste :)

As you say, a matter of style and taste.

> > 2. you have replaced the cond_signal with a cond_broadcast in the
> > enqueing function
> 
> > The first change should make no difference (it is just moving the actual
> > check to another place). The second will cause more threads to wake-up,
> > quite often more than are required. I must admit am not at all clear how
> > should this help.
> 
> This will prevent lost-wakeups. So instead of waking-up one thread, You wake
them all up.
> 
> I also made an error in my code. After every dequeue() I signaled This is not
necessary. I attached my new pound.c file.

No error. Signals are lost if no thread is waiting. If you don't signal
you may well end up with things on the queue and all threads sleeping.
For example:

- assume 8 worker threads
- you get 12 requests in quick succession
	- the first 8 are consumed by the threads
	- the following 4 are enqueued while the threads are busy
- the 8 threads finish working and go to sleep, waiting for a signal
- 4 requests stay in the queue

This is the reason that the broadcast does not help (the signal is
ignored by active threads) and the signal is necessary on dequeue.
-- 
Robert Segall
Apsis GmbH
Postfach, Uetikon am See, CH-8707
Tel: +41-32-512 30 19


RE: [Pound Mailing List] Queue implementation errors?
Robert Segall <roseg(at)apsis.ch>
2011-03-17 17:45:45 [ SNIP ]
On Mon, 2011-03-07 at 14:57 +0000, Joe Gooch wrote:
> As I see it this is the most important change:
> >     (void)pthread_mutex_lock(&arg_mut);
> > 
> >     res->next = NULL;
> > 
> >     if (last == NULL) {
> 
> 
> res->next isn't set to null in the 2.6c code.
> 
> Joe

Thanks, excellent catch. Fixed and will be part of 2.6d
-- 
Robert Segall
Apsis GmbH
Postfach, Uetikon am See, CH-8707
Tel: +41-32-512 30 19


RE: [Pound Mailing List] Queue implementation errors?
Steven van der Vegt <steven(at)echelon.nl>
2011-03-23 15:35:42 [ SNIP ]
First, I don't want this discussion to become a "Yes it is. No it isn't" one.
As long as you too think this is an interesting conversation, I will try to
explain my code :)

> No error. Signals are lost if no thread is waiting. If you don't signal
> you may well end up with things on the queue and all threads sleeping.
> For example:

I think your example is incorrect, because:

> - assume 8 worker threads
> - you get 12 requests in quick succession
>	- the first 8 are consumed by the threads
>	- the following 4 are enqueued while the threads are busy

Correct so far...

> - the 8 threads finish working and go to sleep, waiting for a signal

The only line which can put a thread to sleep is the following one in the
get_thr_arg():

while (first == NULL)
        (void)pthread_cond_wait(&arg_cond, &arg_mut);

This only happens if the queue is empty, with 4 queued items, this is not the
case.

> - 4 requests stay in the queue
>
> This is the reason that the broadcast does not help (the signal is
> ignored by active threads) and the signal is necessary on dequeue.

The only moment a thread has to be wake-upped is when the queue is empty (thus,
all threads are sleeping) and an item is added. If a item is added and the
queue is not empty, apparently all threads are running. They will check later
for that newly added item.

If you are not sure about the solution, please check it with a verification
tool like SPIN. As far as I can see, it is safe, and more efficient than the
current one.


Kind regards,

Steven

-----Oorspronkelijk bericht-----
Van: Robert Segall [mailto:roseg(at)apsis.ch] 
Verzonden: donderdag 17 maart 2011 17:45
Aan: pound(at)apsis.ch
Onderwerp: RE: [Pound Mailing List] Queue implementation errors?

On Wed, 2011-03-09 at 10:54 +0000, Steven van der Vegt wrote:
> > From what I see, you have made two changes:
> 
> > 1. you have moved the NULL check from the caller in http.c to the getter
> > in pound.c (a NULL check is required in any case, as the condition
> > wake-up may occur even when there is no data to be consumed)
> 
> This is true, but why place this responsibility by the caller? This
get_thr_arg is meant to be partial right? Now it is not. Also I think the code
is more clear now, but that's of course a matter of taste :)

As you say, a matter of style and taste.

> > 2. you have replaced the cond_signal with a cond_broadcast in the
> > enqueing function
> 
> > The first change should make no difference (it is just moving the actual
> > check to another place). The second will cause more threads to wake-up,
> > quite often more than are required. I must admit am not at all clear how
> > should this help.
> 
> This will prevent lost-wakeups. So instead of waking-up one thread, You wake
them all up.
> 
> I also made an error in my code. After every dequeue() I signaled This is not
necessary. I attached my new pound.c file.

No error. Signals are lost if no thread is waiting. If you don't signal
you may well end up with things on the queue and all threads sleeping.
For example:

- assume 8 worker threads
- you get 12 requests in quick succession
	- the first 8 are consumed by the threads
	- the following 4 are enqueued while the threads are busy
- the 8 threads finish working and go to sleep, waiting for a signal
- 4 requests stay in the queue

This is the reason that the broadcast does not help (the signal is
ignored by active threads) and the signal is necessary on dequeue.
-- 
Robert Segall
Apsis GmbH
Postfach, Uetikon am See, CH-8707
Tel: +41-32-512 30 19


--
To unsubscribe send an email with subject unsubscribe to pound(at)apsis.ch.
Please contact roseg(at)apsis.ch for questions.

MailBoxer