Hi Luca, [better/easier to talk about details on dev@]
On Fri, Jun 30, 2017 at 11:05 AM, <bugzi...@apache.org> wrote: > https://bz.apache.org/bugzilla/show_bug.cgi?id=60956 > > --- Comment #11 from Luca Toscano <toscano.l...@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?
