Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c

Ruediger Pluem


On 07/03/2019 03:46 PM, [hidden email] wrote:

> Author: icing
> Date: Wed Jul  3 13:46:31 2019
> New Revision: 1862475
>
> URL: http://svn.apache.org/viewvc?rev=1862475&view=rev
> Log:
>   *) mod_http2/mpm_event: Fixes the behaviour when a HTTP/2 connection has nothing
>      more to write with streams ongoing (flow control block). The timeout waiting
>      for the client to send WINODW_UPDATE was incorrectly KeepAliveTimeout and not
>      Timeout as it should be. Fixes PR 63534. [Yann Ylavic, Stefan Eissing]
>
>
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/modules/http2/h2_conn.c
>     httpd/httpd/trunk/modules/http2/h2_version.h
>     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=1862475&r1=1862474&r2=1862475&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Wed Jul  3 13:46:31 2019
> @@ -1153,10 +1153,11 @@ read_request:
>          else if (ap_filter_should_yield(c->output_filters)) {
>              pending = OK;
>          }
> -        if (pending == OK) {
> +        if (pending == OK || (pending == DECLINED &&
> +                              cs->pub.sense == CONN_SENSE_WANT_READ)) {

Sorry for being devils advocate here and I may be completely off, but are we sure that
cs->pub.sense is still CONN_SENSE_WANT_READ when we get here in all cases?
If my memory is correct the cs->pub.sense with CONN_SENSE_WANT_READ / CONN_SENSE_WANT_WRITE
was introduced as HTTP state might be writing or reading, but the underlying SSL needs just the
opposite due to the state the SSL protocol is in.
To be honest I did not have the time to dig deeper, but without further research it might happen
that mod_ssl flips this around to CONN_SENSE_WANT_WRITE.
More than happy to be proven wrong :-)

Regards

RĂ¼diger
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c

Yann Ylavic
On Wed, Jul 3, 2019 at 6:38 PM Ruediger Pluem <[hidden email]> wrote:
>
> On 07/03/2019 03:46 PM, [hidden email] wrote:
> >
> > -        if (pending == OK) {
> > +        if (pending == OK || (pending == DECLINED &&
> > +                              cs->pub.sense == CONN_SENSE_WANT_READ)) {
>
> Sorry for being devils advocate here and I may be completely off, but are we sure that
> cs->pub.sense is still CONN_SENSE_WANT_READ when we get here in all cases?

If CONN_SENSE_WANT_READ is set by the core or a module, it won't
change "inside" the MPM until after READ/WRITE_COMPLETION has been
handled, i.e. precisely in this "if {" block where it's reset to
default.

> If my memory is correct the cs->pub.sense with CONN_SENSE_WANT_READ / CONN_SENSE_WANT_WRITE
> was introduced as HTTP state might be writing or reading, but the underlying SSL needs just the
> opposite due to the state the SSL protocol is in.

Frankly I don't see how the CONN_SENSE_WANT_READ set in
ssl_filter_write (i.e. ssl_io_filter_output) can ever reach MPM event
in any meaningful way. The mod_ssl output filter runs quite late in
the connection processing, well after the handshake anyway, and any
error raised from there leads to the connection to be closed (EAGAIN
is not something we handle in the output filtering stack).
If we wanted the MPM to wait for readability during the SSL handshake
then ssl_hook_process_connection would have to return something else
than DECLINED in any case, e.g. OK when its ap_get_brigade() =>
ssl_io_filter_input() => ssl_io_filter_handshake() => SSL_accept()
returns EAGAIN (with CONN_STATE_WRITE_COMPLETION and
CONN_SENSE_WANT_READ/WRITE set appropriately, see attached trunk/2.4.x
patches).

Thus for me the current CONN_SENSE_WANT_READ is useless so this change
(r1862475) can't break it (and looks like the right thing to do to
handle CONN_SENSE_WANT_READ in MPM event).

> To be honest I did not have the time to dig deeper, but without further research it might happen
> that mod_ssl flips this around to CONN_SENSE_WANT_WRITE.
> More than happy to be proven wrong :-)

I'm not 100% sure to be right but it looks like CONN_SENSE_* semantics
have gone with the different places we have handled SSL handshake in
time..


Regards,
Yann.

async_handshake-trunk.diff (6K) Download Attachment
async_handshake-2.4.x.diff (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c

Ruediger Pluem


On 07/03/2019 11:13 PM, Yann Ylavic wrote:

> On Wed, Jul 3, 2019 at 6:38 PM Ruediger Pluem <[hidden email]> wrote:
>>
>> On 07/03/2019 03:46 PM, [hidden email] wrote:
>>>
>>> -        if (pending == OK) {
>>> +        if (pending == OK || (pending == DECLINED &&
>>> +                              cs->pub.sense == CONN_SENSE_WANT_READ)) {
>>
>> Sorry for being devils advocate here and I may be completely off, but are we sure that
>> cs->pub.sense is still CONN_SENSE_WANT_READ when we get here in all cases?
>
> If CONN_SENSE_WANT_READ is set by the core or a module, it won't
> change "inside" the MPM until after READ/WRITE_COMPLETION has been
> handled, i.e. precisely in this "if {" block where it's reset to
> default.
>
>> If my memory is correct the cs->pub.sense with CONN_SENSE_WANT_READ / CONN_SENSE_WANT_WRITE
>> was introduced as HTTP state might be writing or reading, but the underlying SSL needs just the
>> opposite due to the state the SSL protocol is in.
>
> Frankly I don't see how the CONN_SENSE_WANT_READ set in
> ssl_filter_write (i.e. ssl_io_filter_output) can ever reach MPM event
> in any meaningful way. The mod_ssl output filter runs quite late in
> the connection processing, well after the handshake anyway, and any
> error raised from there leads to the connection to be closed (EAGAIN
> is not something we handle in the output filtering stack).
> If we wanted the MPM to wait for readability during the SSL handshake
> then ssl_hook_process_connection would have to return something else
> than DECLINED in any case, e.g. OK when its ap_get_brigade() =>
> ssl_io_filter_input() => ssl_io_filter_handshake() => SSL_accept()
> returns EAGAIN (with CONN_STATE_WRITE_COMPLETION and
> CONN_SENSE_WANT_READ/WRITE set appropriately, see attached trunk/2.4.x
> patches).
>
> Thus for me the current CONN_SENSE_WANT_READ is useless so this change
> (r1862475) can't break it (and looks like the right thing to do to
> handle CONN_SENSE_WANT_READ in MPM event).
>
>> To be honest I did not have the time to dig deeper, but without further research it might happen
>> that mod_ssl flips this around to CONN_SENSE_WANT_WRITE.
>> More than happy to be proven wrong :-)
>
> I'm not 100% sure to be right but it looks like CONN_SENSE_* semantics
> have gone with the different places we have handled SSL handshake in
> time..

Thanks for the investigation. Wouldn't that bio_filter_out_flush in the patch cause a blocking write, although we were
non blocking?

Regards

RĂ¼diger
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1862475 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_conn.c modules/http2/h2_version.h server/mpm/event/event.c

Yann Ylavic
On Fri, Jul 5, 2019 at 10:37 AM Ruediger Pluem <[hidden email]> wrote:
>
> Wouldn't that bio_filter_out_flush in the patch cause a blocking write, although we were
> non blocking?

That's what we already do in ssl_io_filter_output() (e.g. EAGAIN from
reading the bucket just above).
We never set (only clear) the bio_retry_flag in the bio_filter_out_*()
functions which always flush, so I don't thing that openssl will ever
return WANT_WRITE from SSL_write() with our mechanism.
Anyway, if it happens (!) we are allowed to block here in the output
filter, so FLUSH-ing is probably what makes more sense directly in
this worker thread.
A prossibly better option in trunk (and its pending/aside data
mechanism) would be to break out and leave this bucket with the aside
ones, then let WRITE_COMPLETION do the job when possible. Not for
2.4.x though..

Regards,
Yann.