httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: [Bug 60956] Event MPM listener thread may get blocked by SSL shutdowns
Date Fri, 30 Jun 2017 10:18:59 GMT
Hi Luca,

[better/easier to talk about details on dev@]

On Fri, Jun 30, 2017 at 11:05 AM,  <bugzilla@apache.org> wrote:
> https://bz.apache.org/bugzilla/show_bug.cgi?id=60956
>
> --- Comment #11 from Luca Toscano <toscano.luca@gmail.com> ---
> Other two interesting trunk improvements that have not been backported yet:
>
> http://svn.apache.org/viewvc?view=revision&revision=1706669
> http://svn.apache.org/viewvc?view=revision&revision=1734656
>
> IIUC these ones are meant to provide a more async behavior to most of the
> output filters, namely setting aside buckets (on the heap) to avoid blocking.

These are quite orthogonal I think, and don't seem to fix this particular issue.

>
> After a bit of thinking it seems to me that we'd need to find a solution that
> prevents the mod_ssl_output filter to block, but in a safe way.

IMHO mod_ssl shoudn't (BIO_)flush unconditionally in
modssl_smart_shutdown(), only in the "abortive" mode of
ssl_filter_io_shutdown().

The caller of the ssl output filters should decide when to flush, so
http://svn.apache.org/r1651077 was not the good fix in this regard.

Even if we don't BIO_flush, openssl shouldn't retain the close-notify
by itself, so at least it should go down to the next/core ouput filter
(and stay there until the socket is write-able or asked to flush).

>
> In this particular case we assume this about start_lingering_close_nonblocking:
>
> """
> /*
>  * Close our side of the connection, NOT flushing data to the client.
>  * This should only be called if there has been an error or if we know
>  * that our send buffers are empty.
>  * Pre-condition: cs is not in any timeout queue and not in the pollset,
>  *                timeout_mutex is not locked
>  * return: 0 if connection is fully closed,
>  *         1 if connection is lingering
>  * may be called by listener thread
>  */
> """
>
> I tried the following patch:
>
> """
> Index: server/mpm/event/event.c
> ===================================================================
> --- server/mpm/event/event.c    (revision 1800362)
> +++ server/mpm/event/event.c    (working copy)
> @@ -744,10 +744,7 @@
>      conn_rec *c = cs->c;
>      apr_socket_t *csd = cs->pfd.desc.s;
>
> -    if (ap_prep_lingering_close(c)
> -        || c->aborted
> -        || ap_shutdown_conn(c, 0) != APR_SUCCESS || c->aborted
> -        || apr_socket_shutdown(csd, APR_SHUTDOWN_WRITE) != APR_SUCCESS) {
> +    if (ap_prep_lingering_close(c) || c->aborted) {
>          apr_socket_close(csd);
>          ap_push_pool(worker_queue_info, cs->p);
>          if (dying)
> """
>
> So the idea was to brutally close the connection only if
> ap_prep_lingering_close(c) is not 0 or if the client has already aborted, but
> to leave all the other cases to the start_lingering_close_common. This is
> probably not enough/correct because the connection would go into the
> lingering_close queue, to be picked up again by
> process_timeout_queue(linger_q,..) after the timeout that would call
> stop_lingering_close, that would in turn simply close the socket without giving
> the possibility to mod_ssl to flush its close-notify (because no
> ap_shutdown_conn would be called).

With a possibly non-blocking modssl_smart_shutdown(), I think we could
make ap_shutdown_conn(c, 0) return something like APR_INCOMPLETE for
the case the shutdown was "buffered" in the output filter stack (e.g.
core output filter).

In mpm_event, we would then go to (or stay in) the WRITE_COMPLETION
state instead of LINGER, until every remaining piece data is flushed
successfully.

Does this sound OK?

Mime
View raw message