Re: Livelock bug in conn_pool_get()/http.c

Hi Paul,


Paul Keogh schrieb:
> Hi,
> 
> I've experienced a livelock (ie. CPU bound) bug in the conn_pool_get()
> function in http.c.
> 
> I'm sending lots of HTTP requests to a server that closes the connection
> after every request.
> After 5/6/7 requests the conn_pool_get() function gets stuck in an
> infinite look and never returns;

hmm, this should be something different.

> 
>>From looking at the code;
> 
>     do {
>         retry = 0;
	ˆˆˆˆˆˆˆˆˆˆˆˆ
here we reset retry... and when no conns in dict found than no retry 
will be performed.

>         key = conn_pool_key(host, port);
>         mutex_lock(conn_pool_lock);
>         list = dict_get(conn_pool, key);
>         if (list != NULL)
>             conn = gwlist_extract_first(list);
>         mutex_unlock(conn_pool_lock);
>         /*
>          * Note: we don't hold conn_pool_lock when we
> check/destroy/unregister
>          *       connection because otherwise we can deadlock! And it's
> even better
>          *       not to delay other threads while we check connection.
>          */
>         if (conn != NULL) {
> #ifdef USE_KEEPALIVE
>             /* unregister our server disconnect callback */
>             conn_unregister(conn);
> #endif 
>             /*
>              * Check whether the server has closed the connection while
>              * it has been in the pool.
>              */
>             conn_wait(conn, 0);
>             if (conn_eof(conn) || conn_error(conn)) {
>                 debug("gwlib.http", 0, "HTTP:conn_pool_get: Server
> closed connection, destroying it <%s><%p><fd:%d>.",
>                       octstr_get_cstr(key), conn, conn_get_id(conn));
>                 conn_destroy(conn);
>                 retry = 1;
>                 conn = NULL;
>             }
>         }
>         octstr_destroy(key);
>     } while(retry == 1);
> 
> * The retry variable is set to 1 because the server has closed the
> connection
> 

right

> * This forces the while loop to run

right

> 
> * There are no conns in the list for this host/port combination so conn
> is always NULL subsequently. The
> retry variable is not reset to 0 on this condition and the loop locks
> up.

not true. see above...

> 
> * One solution is to reset retry to 0 on a NULL conn - 
> 
>         if (conn != NULL) {
> #ifdef USE_KEEPALIVE
> 		....
> 	  }
> 	  else
>             retry = 0;
> 
> This does appear to fix the issue in my test harness.
> 
> Comments ?
> 

I don't know why this change should help. could you please try debug it 
further?

> 
> 
> 
> 


Thanks,
Alex