You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2016/01/04 16:57:14 UTC
Re: svn commit: r1722899 - in /httpd/httpd/trunk: ./ modules/http2/
On Mon, Jan 4, 2016 at 4:30 PM, <ic...@apache.org> 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.
Re: svn commit: r1722899 - in /httpd/httpd/trunk: ./ modules/http2/
Posted by Stefan Eissing <st...@greenbytes.de>.
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 <yl...@gmail.com>:
>
> On Mon, Jan 4, 2016 at 4:30 PM, <ic...@apache.org> 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.