Re: svn commit: r1538490 - in /httpd/httpd/trunk/server/mpm: event/event.c eventopt/eventopt.c

[Jeff Trawick <tr...@gmail.com>]

On Sun, Nov 3, 2013 at 8:37 PM, <co...@apache.org> wrote:

> Author: covener
> Date: Mon Nov  4 01:37:31 2013
> New Revision: 1538490
>
> URL: http://svn.apache.org/r1538490
> Log:
> c->sbh can be unexpectedly NULL when the thread that pulls the ready
> keepalive
> connection out of the queue laps the thread that put it on the queue.
>
>
> Modified:
>     httpd/httpd/trunk/server/mpm/event/event.c
>     httpd/httpd/trunk/server/mpm/eventopt/eventopt.c
>
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1538490&r1=1538489&r2=1538490&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Mon Nov  4 01:37:31 2013
> @@ -1066,6 +1066,7 @@ read_request:
>           */
>          cs->expiration_time = ap_server_conf->keep_alive_timeout +
>                                apr_time_now();
> +        c->sbh = NULL;
>          apr_thread_mutex_lock(timeout_mutex);
>          TO_QUEUE_APPEND(keepalive_q, cs);
>
> @@ -1079,6 +1080,7 @@ read_request:
>                           "process_socket: apr_pollset_add failure");
>              AP_DEBUG_ASSERT(rc == APR_SUCCESS);
>          }
> +        return;
>      }
>      else if (cs->pub.state == CONN_STATE_SUSPENDED) {
>          apr_atomic_inc32(&suspended_count);
>

May as well clear sbh in other places where  process_socket() returns,
right?

(The notify_suspend()/notify_resume() which corresponds to clearing/setting
sbh is just a sketch for a new hook; it seems that, the thread
notifications should match clearing/setting/sbh inside process_socket().)

Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c (revision 1545642)
+++ server/mpm/event/event.c (working copy)
@@ -976,6 +976,7 @@
     else {
         c = cs->c;
         c->sbh = sbh;
+        notify_resume(cs);
         c->current_thread = thd;
     }

@@ -1028,6 +1029,8 @@
              * event thread poll for writeability.
              */
             cs->expiration_time = ap_server_conf->timeout + apr_time_now();
+            c->sbh = NULL;
+            notify_suspend(cs);
             apr_thread_mutex_lock(timeout_mutex);
             TO_QUEUE_APPEND(write_completion_q, cs);
             cs->pfd.reqevents = (
@@ -1052,8 +1055,11 @@
     }

     if (cs->pub.state == CONN_STATE_LINGER) {
-        if (!start_lingering_close_blocking(cs))
+        if (!start_lingering_close_blocking(cs)) {
+            c->sbh = NULL;
+            notify_suspend(cs);
             return;
+        }
     }
     else if (cs->pub.state == CONN_STATE_CHECK_REQUEST_LINE_READABLE) {
         /* It greatly simplifies the logic to use a single timeout value
here
@@ -1067,6 +1073,7 @@
         cs->expiration_time = ap_server_conf->keep_alive_timeout +
                               apr_time_now();
         c->sbh = NULL;
+        notify_suspend(cs);
         apr_thread_mutex_lock(timeout_mutex);
         TO_QUEUE_APPEND(keepalive_q, cs);

@@ -1092,6 +1099,7 @@
      * or timeout.
      */
     c->sbh = NULL;
+    notify_suspend(cs);
     return;
 }


> Modified: httpd/httpd/trunk/server/mpm/eventopt/eventopt.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/eventopt/eventopt.c?rev=1538490&r1=1538489&r2=1538490&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/eventopt/eventopt.c (original)
> +++ httpd/httpd/trunk/server/mpm/eventopt/eventopt.c Mon Nov  4 01:37:31
> 2013
> @@ -1124,7 +1124,7 @@ read_request:
>           */
>          cs->expiration_time = ap_server_conf->keep_alive_timeout +
>                                apr_time_now();
> -
> +        c->sbh = NULL;
>          /* Add work to pollset. */
>          v = ap_equeue_writer_value(eq);
>          v->timeout_type = TIMEOUT_KEEPALIVE;
> @@ -1133,6 +1133,7 @@ read_request:
>          v->tag = "process_socket(keepalive)";
>          ap_equeue_writer_onward(eq);
>          apr_pollset_wakeup(event_pollset);
> +        return;
>      }
>      else if (cs->pub.state == CONN_STATE_SUSPENDED) {
>          apr_atomic_inc32(&suspended_count);
>
>
>


-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: svn commit: r1538490 - in /httpd/httpd/trunk/server/mpm: event/event.c eventopt/eventopt.c

[Eric Covener <co...@gmail.com>]

> May as well clear sbh in other places where  process_socket() returns,
> right?

+1, I think I was zoomed in on the ones that didn't return and would
hit the c->sbh = NULL at the bottom of the method.