Thanks, Yann. Those are leftovers from an earlier age and it's good that you spotted those. Might explain some people complaining about loss of server threads / workers after some time if h2 session shutdown waits on a lock not released that way.
> Am 04.01.2016 um 16:57 schrieb Yann Ylavic <[email protected]>: > > On Mon, Jan 4, 2016 at 4:30 PM, <[email protected]> wrote: >> Author: icing >> Date: Mon Jan 4 15:30:36 2016 >> New Revision: 1722899 >> >> URL: http://svn.apache.org/viewvc?rev=1722899&view=rev >> Log: >> reworked synching of session shutdown with worker threads, workers now stick >> to a session until no more reuqquest are tbd, keepalive handling revisited >> > [] >> >> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c >> URL: >> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1722899&r1=1722898&r2=1722899&view=diff >> ============================================================================== >> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original) >> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Mon Jan 4 15:30:36 2016 > [] >> @@ -700,33 +694,25 @@ apr_status_t h2_mplx_out_write(h2_mplx * >> { >> apr_status_t status; >> AP_DEBUG_ASSERT(m); >> - if (m->aborted) { >> - return APR_ECONNABORTED; >> - } >> status = apr_thread_mutex_lock(m->lock); >> if (APR_SUCCESS == status) { >> - if (!m->aborted) { >> - h2_io *io = h2_io_set_get(m->stream_ios, stream_id); >> - if (io && !io->orphaned) { >> - status = out_write(m, io, f, bb, trailers, iowait); >> - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c, >> - "h2_mplx(%ld-%d): write with trailers=%s", >> - m->id, io->id, trailers? "yes" : "no"); >> - H2_MPLX_IO_OUT(APLOG_TRACE2, m, io, "h2_mplx_out_write"); >> - >> - have_out_data_for(m, stream_id); >> - if (m->aborted) { >> - return APR_ECONNABORTED; >> - } >> - } >> - else { >> - status = APR_ECONNABORTED; >> + h2_io *io = h2_io_set_get(m->stream_ios, stream_id); >> + if (io && !io->orphaned) { >> + status = out_write(m, io, f, bb, trailers, iowait); >> + ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c, >> + "h2_mplx(%ld-%d): write with trailers=%s", >> + m->id, io->id, trailers? "yes" : "no"); >> + H2_MPLX_IO_OUT(APLOG_TRACE2, m, io, "h2_mplx_out_write"); >> + >> + have_out_data_for(m, stream_id); >> + if (m->aborted) { >> + return APR_ECONNABORTED; > > No apr_thread_mutex_unlock(m->lock) before leaving here? > >> } >> } >> - >> - if (m->lock) { >> - apr_thread_mutex_unlock(m->lock); >> + else { >> + status = APR_ECONNABORTED; >> } >> + apr_thread_mutex_unlock(m->lock); >> } >> return status; >> } >> @@ -735,44 +721,39 @@ apr_status_t h2_mplx_out_close(h2_mplx * >> { >> apr_status_t status; >> AP_DEBUG_ASSERT(m); >> - if (m->aborted) { >> - return APR_ECONNABORTED; >> - } >> status = apr_thread_mutex_lock(m->lock); >> if (APR_SUCCESS == status) { >> - if (!m->aborted) { >> - h2_io *io = h2_io_set_get(m->stream_ios, stream_id); >> - if (io && !io->orphaned) { >> - if (!io->response && !io->rst_error) { >> - /* In case a close comes before a response was created, >> - * insert an error one so that our streams can properly >> - * reset. >> - */ >> - h2_response *r = h2_response_die(stream_id, >> APR_EGENERAL, >> - io->request, m->pool); >> - status = out_open(m, stream_id, r, NULL, NULL, NULL); >> - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c, >> - "h2_mplx(%ld-%d): close, no response, no >> rst", >> - m->id, io->id); >> - } >> + h2_io *io = h2_io_set_get(m->stream_ios, stream_id); >> + if (io && !io->orphaned) { >> + if (!io->response && !io->rst_error) { >> + /* In case a close comes before a response was created, >> + * insert an error one so that our streams can properly >> + * reset. >> + */ >> + h2_response *r = h2_response_die(stream_id, APR_EGENERAL, >> + io->request, m->pool); >> + status = out_open(m, stream_id, r, NULL, NULL, NULL); >> ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c, >> - "h2_mplx(%ld-%d): close with trailers=%s", >> - m->id, io->id, trailers? "yes" : "no"); >> - status = h2_io_out_close(io, trailers); >> - H2_MPLX_IO_OUT(APLOG_TRACE2, m, io, "h2_mplx_out_close"); >> - >> - have_out_data_for(m, stream_id); >> - if (m->aborted) { >> - /* if we were the last output, the whole session might >> - * have gone down in the meantime. >> - */ >> - return APR_SUCCESS; >> - } >> + "h2_mplx(%ld-%d): close, no response, no rst", >> + m->id, io->id); >> } >> - else { >> - status = APR_ECONNABORTED; >> + ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c, >> + "h2_mplx(%ld-%d): close with trailers=%s", >> + m->id, io->id, trailers? "yes" : "no"); >> + status = h2_io_out_close(io, trailers); >> + H2_MPLX_IO_OUT(APLOG_TRACE2, m, io, "h2_mplx_out_close"); >> + >> + have_out_data_for(m, stream_id); >> + if (m->aborted) { >> + /* if we were the last output, the whole session might >> + * have gone down in the meantime. >> + */ >> + return APR_SUCCESS; > > Likewise? > >> } >> } >> + else { >> + status = APR_ECONNABORTED; >> + } >> apr_thread_mutex_unlock(m->lock); >> } >> return status; > > > Regards, > Yann.
