Re: async mod_proxy_http
On Thu, Sep 13, 2018 at 1:59 AM Yann Ylavic wrote: > > On Wed, Sep 12, 2018 at 5:53 PM Eric Covener wrote: > > > > Forking from the Cool Stuff thread. > > > > Have you noticed that the wstunnell stuff makes the suspended count in > > the MPM grow? There is no API for us to tell the MPM that when we get > > the socket-activity callback that we are "resuming" something. > > > > (going from vague recollection) > > It seems that we increment it once when the handler returns SUSPENDED, > and decrement it once per connection too in proxy_wstunnel_finish(). Besides this, don't we want suspended/resumed connection to have their scoreboard status change? No particular idea of how we'd do this but looks sensible to me to be able to see how many suspended (or not) connections there are. Regards, Yann.
Re: async mod_proxy_http
On Thu, Sep 13, 2018 at 10:55 AM Plüm, Rüdiger, Vodafone Group wrote: > > > -Ursprüngliche Nachricht- > > Von: Yann Ylavic > > Gesendet: Donnerstag, 13. September 2018 10:37 > > An: httpd-dev > > Betreff: Re: async mod_proxy_http > > > > On Thu, Sep 13, 2018 at 8:49 AM Plüm, Rüdiger, Vodafone Group > > wrote: > > > > > > I don't like the "misuse" of c->aborted here. I for instance log in > > > the access log whether connections have been aborted or not and this > > > approach would mean that all proxied websocket connections would get > > > marked as aborted. Can't we use any other flag to tell the MPM to > > > close the socket and push the pool, e.g. a note in c->notes? > > > Why is the lingering close no longer needed? > > > > Agreed, let lingering close do its job if the client connection is not > > closed already. > > Better in v2 (attached)? > > Better. Should a module outside the core directly fiddle around with > the connection state in this case setting it to CONN_STATE_LINGER? I'd said no... but for modules playing async with the MPM :p One way or another we need a flag which is meant for the MPM at resume_suspended time (be it the state, a c->notes, ...), and that hook is likely to be called by modules going async... Regards, Yann. > > > > > > Why now doing ap_mpm_resume_suspended after > > > ap_finalize_request_protocol(baton->r) and > > > ap_process_request_after_handler(baton->r)? > > > > I think we don't want EOS/EOR filtering race with the MPM on the > > connection... > > > > Fair enough. > > Regards > > Rüdiger >
Re: async mod_proxy_http
On Thu, Sep 13, 2018 at 8:49 AM Plüm, Rüdiger, Vodafone Group
wrote:
>
> I don't like the "misuse" of c->aborted here. I for instance log in
> the access log whether connections have been aborted or not and this
> approach would mean that all proxied websocket connections would get
> marked as aborted. Can't we use any other flag to tell the MPM to
> close the socket and push the pool, e.g. a note in c->notes?
> Why is the lingering close no longer needed?
Agreed, let lingering close do its job if the client connection is not
closed already.
Better in v2 (attached)?
> Why now doing ap_mpm_resume_suspended after
> ap_finalize_request_protocol(baton->r) and
> ap_process_request_after_handler(baton->r)?
I think we don't want EOS/EOR filtering race with the MPM on the connection...
Regards,
Yann.
Index: modules/proxy/mod_proxy_wstunnel.c
===
--- modules/proxy/mod_proxy_wstunnel.c (revision 1840709)
+++ modules/proxy/mod_proxy_wstunnel.c (working copy)
@@ -155,15 +155,14 @@ static int proxy_wstunnel_pump(ws_baton_t *baton,
static void proxy_wstunnel_finish(ws_baton_t *baton)
{
+conn_rec *c = baton->r->connection;
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, baton->r, "proxy_wstunnel_finish");
baton->proxy_connrec->close = 1; /* new handshake expected on each back-conn */
-baton->r->connection->keepalive = AP_CONN_CLOSE;
ap_proxy_release_connection(baton->scheme, baton->proxy_connrec, baton->r->server);
-ap_finalize_request_protocol(baton->r);
-ap_lingering_close(baton->r->connection);
-apr_socket_close(baton->client_soc);
-ap_mpm_resume_suspended(baton->r->connection);
+ap_finalize_request_protocol(baton->r); /* send EOS */
ap_process_request_after_handler(baton->r); /* don't touch baton or r after here */
+c->cs->state = CONN_STATE_LINGER;
+ap_mpm_resume_suspended(c);
}
/* If neither socket becomes readable in the specified timeout,
Index: server/mpm/event/event.c
===
--- server/mpm/event/event.c (revision 1840709)
+++ server/mpm/event/event.c (working copy)
@@ -1273,6 +1273,14 @@ static apr_status_t event_resume_suspended (conn_r
apr_atomic_dec32(_count);
c->suspended_baton = NULL;
+if (cs->pub.state == CONN_STATE_LINGER) {
+if (start_lingering_close_blocking(cs) == OK) {
+process_lingering_close(cs);
+}
+return OK;
+}
+
+cs->pub.state = CONN_STATE_WRITE_COMPLETION;
cs->queue_timestamp = apr_time_now();
cs->pfd.reqevents = (
cs->pub.sense == CONN_SENSE_WANT_READ ? APR_POLLIN :
Re: async mod_proxy_http
On Wed, Sep 12, 2018 at 7:59 PM Yann Ylavic wrote: > > On Wed, Sep 12, 2018 at 5:53 PM Eric Covener wrote: > > > > Forking from the Cool Stuff thread. > > > > Have you noticed that the wstunnell stuff makes the suspended count in > > the MPM grow? There is no API for us to tell the MPM that when we get > > the socket-activity callback that we are "resuming" something. > > > > (going from vague recollection) > > It seems that we increment it once when the handler returns SUSPENDED, > and decrement it once per connection too in proxy_wstunnel_finish(). This didn't even ring a bell :) But won't we return SUSPENDED many times to a single finish? > > However, looks like there is unnecessary churn > proxy_wstunnel_finish(), including a double close since the MPM will > also finally close the client connection. How about something like the > attached? Looks reasonable.
Re: async mod_proxy_http
On Wed, Sep 12, 2018 at 5:53 PM Eric Covener wrote:
>
> Forking from the Cool Stuff thread.
>
> Have you noticed that the wstunnell stuff makes the suspended count in
> the MPM grow? There is no API for us to tell the MPM that when we get
> the socket-activity callback that we are "resuming" something.
>
> (going from vague recollection)
It seems that we increment it once when the handler returns SUSPENDED,
and decrement it once per connection too in proxy_wstunnel_finish().
However, looks like there is unnecessary churn
proxy_wstunnel_finish(), including a double close since the MPM will
also finally close the client connection. How about something like the
attached?
Regards,
Yann.
Index: modules/proxy/mod_proxy_wstunnel.c
===
--- modules/proxy/mod_proxy_wstunnel.c (revision 1840709)
+++ modules/proxy/mod_proxy_wstunnel.c (working copy)
@@ -155,15 +155,14 @@ static int proxy_wstunnel_pump(ws_baton_t *baton,
static void proxy_wstunnel_finish(ws_baton_t *baton)
{
+conn_rec *c = baton->r->connection;
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, baton->r, "proxy_wstunnel_finish");
baton->proxy_connrec->close = 1; /* new handshake expected on each back-conn */
-baton->r->connection->keepalive = AP_CONN_CLOSE;
ap_proxy_release_connection(baton->scheme, baton->proxy_connrec, baton->r->server);
-ap_finalize_request_protocol(baton->r);
-ap_lingering_close(baton->r->connection);
-apr_socket_close(baton->client_soc);
-ap_mpm_resume_suspended(baton->r->connection);
+ap_finalize_request_protocol(baton->r); /* send EOS */
+c->aborted = 1; /* nothing more on the client connection */
ap_process_request_after_handler(baton->r); /* don't touch baton or r after here */
+ap_mpm_resume_suspended(c);
}
/* If neither socket becomes readable in the specified timeout,
Index: server/mpm/event/event.c
===
--- server/mpm/event/event.c (revision 1840709)
+++ server/mpm/event/event.c (working copy)
@@ -1273,6 +1273,12 @@ static apr_status_t event_resume_suspended (conn_r
apr_atomic_dec32(_count);
c->suspended_baton = NULL;
+if (c->aborted) {
+apr_socket_close(ap_get_conn_socket(c));
+ap_queue_info_push_pool(worker_queue_info, cs->p);
+return OK;
+}
+
cs->queue_timestamp = apr_time_now();
cs->pfd.reqevents = (
cs->pub.sense == CONN_SENSE_WANT_READ ? APR_POLLIN :
