Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of ylavic.dev@gmail.com designates
 209.85.223.181 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20141114120446.C8F0523888D2@eris.apache.org>
References: <20141114120446.C8F0523888D2@eris.apache.org>
Date: Tue, 2 Dec 2014 09:09:54 +0100
Message-ID: 
 <CAKQ1sVPneMrHkHvNUwqrZZ2EGua0zfRf0zOzeuz=DzXy35XDNw@mail.gmail.com>
Subject: Re: svn commit: r1639614 -
 /httpd/httpd/trunk/server/mpm/event/event.c
From: Yann Ylavic <ylavic.dev@gmail.com>
To: httpd <dev@httpd.apache.org>
Content-Type: text/plain; charset=UTF-8

On Fri, Nov 14, 2014 at 1:04 PM,  <covener@apache.org> wrote:
> Author: covener
> Date: Fri Nov 14 12:04:46 2014
> New Revision: 1639614
>
> URL: http://svn.apache.org/r1639614
> Log:
> don't call notify_suspend() in a worker thread after
> start_lingering_close_common may have put the socket back
> into the pollset.
>
> If it becomes readable too quickly, cs can be
> free'ed or accessed concurrently.
>
> Modified:
>     httpd/httpd/trunk/server/mpm/event/event.c
>
[]
> +static int start_lingering_close_common(event_conn_state_t *cs, int in_worker)
>  {
>      apr_status_t rv;
>      struct timeout_queue *q;
> @@ -849,6 +861,9 @@ static int start_lingering_close_common(
>              cs->pub.sense == CONN_SENSE_WANT_WRITE ? APR_POLLOUT :
>                      APR_POLLIN) | APR_POLLHUP | APR_POLLERR;
>      cs->pub.sense = CONN_SENSE_DEFAULT;
> +    if (in_worker) {
> +        notify_suspend(cs);
> +    }

Can't we put this outside the critical section?
(should the hook take non negligible time).

>      rv = apr_pollset_add(event_pollset, &cs->pfd);
>      apr_thread_mutex_unlock(timeout_mutex);
>      if (rv != APR_SUCCESS && !APR_STATUS_IS_EEXIST(rv)) {

Eg.

Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c    (revision 1642684)
+++ server/mpm/event/event.c    (working copy)
@@ -859,16 +859,16 @@ static int start_lingering_close_common(event_conn
         cs->pub.state = CONN_STATE_LINGER_NORMAL;
     }
     apr_atomic_inc32(&lingering_count);
+    cs->c->sbh = NULL;
+    if (in_worker) {
+        notify_suspend(cs);
+    }
     apr_thread_mutex_lock(timeout_mutex);
-    cs->c->sbh = NULL;
     TO_QUEUE_APPEND(*q, cs);
     cs->pfd.reqevents = (
             cs->pub.sense == CONN_SENSE_WANT_WRITE ? APR_POLLOUT :
                     APR_POLLIN) | APR_POLLHUP | APR_POLLERR;
     cs->pub.sense = CONN_SENSE_DEFAULT;
-    if (in_worker) {
-        notify_suspend(cs);
-    }
     rv = apr_pollset_add(event_pollset, &cs->pfd);
     apr_thread_mutex_unlock(timeout_mutex);
     if (rv != APR_SUCCESS && !APR_STATUS_IS_EEXIST(rv)) {
--