httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Eissing <stefan.eiss...@greenbytes.de>
Subject Re: svn commit: r1722899 - in /httpd/httpd/trunk: ./ modules/http2/
Date Mon, 04 Jan 2016 16:38:50 GMT
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 <ylavic.dev@gmail.com>:
> 
> On Mon, Jan 4, 2016 at 4:30 PM,  <icing@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.


Mime
View raw message