You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Eissing <st...@eissing.org> on 2017/12/21 17:49:31 UTC
Re: svn commit: r1818960 - /httpd/httpd/trunk/server/mpm/event/event.c
Oops, thought it was OK. That causes the hangers?
> Am 21.12.2017 um 18:45 schrieb ylavic@apache.org:
>
> Author: ylavic
> Date: Thu Dec 21 17:45:08 2017
> New Revision: 1818960
>
> URL: http://svn.apache.org/viewvc?rev=1818960&view=rev
> Log:
> mpm_event: follow up to r1818804.
>
> Allow DONE as a successful ap_run_process_connection() return value, for
> instance h2_conn_run() and h2_task_process_conn() uses it, third-party
> modules may too...
>
>
> Modified:
> httpd/httpd/trunk/server/mpm/event/event.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=1818960&r1=1818959&r2=1818960&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Thu Dec 21 17:45:08 2017
> @@ -1049,10 +1049,16 @@ static void process_socket(apr_thread_t
> apr_atomic_inc32(&clogged_count);
> rc = ap_run_process_connection(c);
> apr_atomic_dec32(&clogged_count);
> + if (rc == DONE) {
> + rc = OK;
> + }
> }
> else if (cs->pub.state == CONN_STATE_READ_REQUEST_LINE) {
> read_request:
> rc = ap_run_process_connection(c);
> + if (rc == DONE) {
> + rc = OK;
> + }
> }
> /*
> * The process_connection hooks above should set the connection state
> @@ -1077,8 +1083,8 @@ read_request:
> * worker or prefork MPMs for instance.
> */
> if (rc != OK || (cs->pub.state != CONN_STATE_LINGER
> - && cs->pub.state != CONN_STATE_CHECK_REQUEST_LINE_READABLE
> && cs->pub.state != CONN_STATE_WRITE_COMPLETION
> + && cs->pub.state != CONN_STATE_CHECK_REQUEST_LINE_READABLE
> && cs->pub.state != CONN_STATE_SUSPENDED)) {
> ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO()
> "process_socket: connection processing %s: closing",
>
>
Re: svn commit: r1818960 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sun, Dec 24, 2017 at 12:06 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
> That one works nicely in the test and load setups! +1
Ok thanks, r1819214.
Re: svn commit: r1818960 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Stefan Eissing <st...@greenbytes.de>.
That one works nicely in the test and load setups! +1
> Am 23.12.2017 um 17:37 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Fri, Dec 22, 2017 at 6:14 PM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>> The changes in h2_h2.c and h2_switch.c work find in my test. However, the
>> change in h2_conn.c which sets CONN_STATE_LINGER does not. It prevents the
>> last GOAWAY frame from the server to be sent out, in some tests.
>>
>> The current flow, when the client sends a GOAWAY is that the session
>> transits to H2_SESSION_ST_DONE, the mpm calls the pre_linger hook and
>> that writes the server's GOAWAY (as it does in situations where the connection
>> is shut down after an idle timeout by the mpm itself).
>
> Argh, forgot about the pre_close_connection hook..
>
>>
>> mod_http2 also returns from connection procession when the connection
>> can only progress when more frames from the client arrive. For example,
>> when the session is H2_SESSION_ST_DONE, which means that all streams have
>> been processed. This is analog to a request being done on a h1 connection.
>>
>> But the h2session and all its state is still alive. So c->aborted should
>> not be set. And it is definitely not in LINGER.
>
> Yes, the issue is probably not about CONN_STATE_LINGER, but c->aborted.
> I wanted to close the connection as quickly as possible, that was not
> a good idea.
> I could replace ap_flush_conn() by ap_lingering_close() in my patch,
> but the simpler is to let the MPM do it, v2 attached simply sets
> CONN_STATE_LINGER (it may be turned to CONN_STATE_WRITE_COMPLETION in
> the Upgrade case, still the MPM will do the right thing with
> c->keepalive = AP_CONN_CLOSE).
> My main concern was returning anything else than
> CONN_STATE_WRITE_COMPLETION or CONN_STATE_LINGER in h2_conn_run(),
> like CONN_STATE_HANDLER.
>
>>
>> I am all for aligning h2 connection states more with the connection state
>> model that mpm_event has (and vice versa). Maybe we have to lock ourselves
>> into a room with a whiteboard and beers and figure this out one day...
>
> Would be a pleasure :)
>
>>
>> Do we have good documentation about the CONN_STATE engine somewhere?
>
> I updated some comments in event.c lately...
> <h2_conn_state-v2.patch>
Re: svn commit: r1818960 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Dec 22, 2017 at 6:14 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> The changes in h2_h2.c and h2_switch.c work find in my test. However, the
> change in h2_conn.c which sets CONN_STATE_LINGER does not. It prevents the
> last GOAWAY frame from the server to be sent out, in some tests.
>
> The current flow, when the client sends a GOAWAY is that the session
> transits to H2_SESSION_ST_DONE, the mpm calls the pre_linger hook and
> that writes the server's GOAWAY (as it does in situations where the connection
> is shut down after an idle timeout by the mpm itself).
Argh, forgot about the pre_close_connection hook..
>
> mod_http2 also returns from connection procession when the connection
> can only progress when more frames from the client arrive. For example,
> when the session is H2_SESSION_ST_DONE, which means that all streams have
> been processed. This is analog to a request being done on a h1 connection.
>
> But the h2session and all its state is still alive. So c->aborted should
> not be set. And it is definitely not in LINGER.
Yes, the issue is probably not about CONN_STATE_LINGER, but c->aborted.
I wanted to close the connection as quickly as possible, that was not
a good idea.
I could replace ap_flush_conn() by ap_lingering_close() in my patch,
but the simpler is to let the MPM do it, v2 attached simply sets
CONN_STATE_LINGER (it may be turned to CONN_STATE_WRITE_COMPLETION in
the Upgrade case, still the MPM will do the right thing with
c->keepalive = AP_CONN_CLOSE).
My main concern was returning anything else than
CONN_STATE_WRITE_COMPLETION or CONN_STATE_LINGER in h2_conn_run(),
like CONN_STATE_HANDLER.
>
> I am all for aligning h2 connection states more with the connection state
> model that mpm_event has (and vice versa). Maybe we have to lock ourselves
> into a room with a whiteboard and beers and figure this out one day...
Would be a pleasure :)
>
> Do we have good documentation about the CONN_STATE engine somewhere?
I updated some comments in event.c lately...
Re: svn commit: r1818960 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 22.12.2017 um 00:31 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Thu, Dec 21, 2017 at 7:04 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Dec 21, 2017 at 6:59 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Thu, Dec 21, 2017 at 6:49 PM, Stefan Eissing <st...@eissing.org> wrote:
>>>> Oops, thought it was OK. That causes the hangers?
>>>
>>> Not anymore :)
>>>
>>> But h2_conn_run() seems to possibly return CONN_STATE_HANDLER, it
>>> looks stange to me, and for once causes hanger ;)
>>> What's the meaning of it?
>>
>> That's for when it's called from h2_protocol_switch() possibly, the
>> issue is that it's also called from h2_h2_process_conn(), a
>> process_connection hook which returns straight to the MPM...
>
> Actually I don't see any other possible state than CONN_STATE_LINGER
> after h2_conn_run(), the master connection has to be terminated in
> both h2_h2_process_conn() hook and
> core_upgrade_handler::ap_switch_protocol() cases.
>
> So how about the attached patch?
> It changes h2_conn_run() to finally/unconditionally set
> CONN_STATE_LINGER after having flushed and aborted the connection
> (aborting prevents further unnecessary actions like
> ap_check_pipeline() or lingering before closing the socket, once the
> connection is flushed...).
> This patch does also s/return DONE/return OK/ where appropriate (the
> DONE in core_upgrade_handler() is fine because we want to stop request
> processing from there).
> <h2_conn_state.patch>
The changes in h2_h2.c and h2_switch.c work find in my test. However, the
change in h2_conn.c which sets CONN_STATE_LINGER does not. It prevents the
last GOAWAY frame from the server to be sent out, in some tests.
The current flow, when the client sends a GOAWAY is that the session
transits to H2_SESSION_ST_DONE, the mpm calls the pre_linger hook and
that writes the server's GOAWAY (as it does in situations where the connection
is shut down after an idle timeout by the mpm itself).
mod_http2 also returns from connection procession when the connection
can only progress when more frames from the client arrive. For example,
when the session is H2_SESSION_ST_DONE, which means that all streams have
been processed. This is analog to a request being done on a h1 connection.
But the h2session and all its state is still alive. So c->aborted should
not be set. And it is definitely not in LINGER.
I am all for aligning h2 connection states more with the connection state
model that mpm_event has (and vice versa). Maybe we have to lock ourselves
into a room with a whiteboard and beers and figure this out one day...
Do we have good documentation about the CONN_STATE engine somewhere?
-Stefan
Re: svn commit: r1818960 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 21, 2017 at 7:04 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Dec 21, 2017 at 6:59 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Dec 21, 2017 at 6:49 PM, Stefan Eissing <st...@eissing.org> wrote:
>>> Oops, thought it was OK. That causes the hangers?
>>
>> Not anymore :)
>>
>> But h2_conn_run() seems to possibly return CONN_STATE_HANDLER, it
>> looks stange to me, and for once causes hanger ;)
>> What's the meaning of it?
>
> That's for when it's called from h2_protocol_switch() possibly, the
> issue is that it's also called from h2_h2_process_conn(), a
> process_connection hook which returns straight to the MPM...
Actually I don't see any other possible state than CONN_STATE_LINGER
after h2_conn_run(), the master connection has to be terminated in
both h2_h2_process_conn() hook and
core_upgrade_handler::ap_switch_protocol() cases.
So how about the attached patch?
It changes h2_conn_run() to finally/unconditionally set
CONN_STATE_LINGER after having flushed and aborted the connection
(aborting prevents further unnecessary actions like
ap_check_pipeline() or lingering before closing the socket, once the
connection is flushed...).
This patch does also s/return DONE/return OK/ where appropriate (the
DONE in core_upgrade_handler() is fine because we want to stop request
processing from there).
Re: svn commit: r1818960 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 21, 2017 at 6:59 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Dec 21, 2017 at 6:49 PM, Stefan Eissing <st...@eissing.org> wrote:
>> Oops, thought it was OK. That causes the hangers?
>
> Not anymore :)
>
> But h2_conn_run() seems to possibly return CONN_STATE_HANDLER, it
> looks stange to me, and for once causes hanger ;)
> What's the meaning of it?
That's for when it's called from h2_protocol_switch() possibly, the
issue is that it's also called from h2_h2_process_conn(), a
process_connection hook which returns straight to the MPM...
Re: svn commit: r1818960 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 21, 2017 at 6:49 PM, Stefan Eissing <st...@eissing.org> wrote:
> Oops, thought it was OK. That causes the hangers?
Not anymore :)
But h2_conn_run() seems to possibly return CONN_STATE_HANDLER, it
looks stange to me, and for once causes hanger ;)
What's the meaning of it?