Re: concurrency issue with http_close_all_ports

Hi David,

thanks for pointing to this race condition!

I can easy reproduce it but I don’t think that your fix is the right one because it may
hung if client don’t read response or read it but very slow.

I have to think a little bit more how to fix it.

Alex	

> Am 31.03.2015 um 11:49 schrieb David Schneider <[email protected]>:
> 
> Hello
> 
> I found a concurrency issue with the latest http server implementation.
> 
> I'm running test_http_server with multiple threads (-t 5). If I send a request which is delayed by a sleep and in the mean time I'm calling "http://localhost:8080/quit" then the server crashes during http_send_reply call due to already freed client->conn.
> 
> Index: test/test_http_server.c
> ===================================================================
> --- test/test_http_server.c    (Revision 5141)
> +++ test/test_http_server.c    (working copy)
> @@ -237,6 +237,9 @@
>         if (extra_headers != NULL)
>             http_header_combine(resph, extra_headers);
> 
> +        if (run)
> +            gwthread_sleep(5);
> +
>         /* return response to client */
>         http_send_reply(client, status, resph, reply_body);
> 
> 
> testing with:
> test/test_http_server -t 5 &
> curl "http://localhost:8080" &
> sleep 1
> curl "http://localhost:8080/quit"
> 
> I found a possible solution in http.c: port_remove
> 
> Index: gwlib/http.c
> ===================================================================
> --- gwlib/http.c    (Revision 5141)
> +++ gwlib/http.c    (working copy)
> @@ -2097,8 +2097,14 @@
>      */
>     gwlist_lock(active_connections);
>     l = gwlist_search_all(active_connections, &port, port_match);
> -    while(l != NULL && (client = gwlist_extract_first(l)) != NULL)
> +    while(l != NULL && (client = gwlist_extract_first(l)) != NULL) {
>         conn_unregister(client->conn);
> +        while (client->state == request_is_being_handled) {
> +            gwthread_sleep(0.1);
> +        }
> +    }
>     gwlist_unlock(active_connections);
>     gwlist_destroy(l, NULL);
>     while((client = gwlist_search(active_connections, &port, port_match)) != NULL)
> 
> 
> What do you think about that solution?
> 
> David
> 
> PS: There is a typo in test_http_server help:
> Index: test/test_http_server.c
> ===================================================================
> --- test/test_http_server.c    (Revision 5141)
> +++ test/test_http_server.c    (working copy)
> @@ -288,7 +291,7 @@
>     info(0, "    read HTTP headers from file 'filename' and add them to");
>     info(0, "    the request for url 'url'");
>     info(0, "specific URIs with special functions are:");
> -    info(0, "  /quite - shutdown the HTTP server");
> +    info(0, "  /quit - shutdown the HTTP server");
>     info(0, "  /whitelist - provides the -w whitelist as response");
>     info(0, "  /blacklist - provides the -b blacklist as response");
>     info(0, "  /save - save a HTTP POST request body to a file /tmp/body.<pid>.<n>");
> 
>