Skip to content

Commit

Permalink
fix for race in connection shutdown
Browse files Browse the repository at this point in the history
  • Loading branch information
Stefan Eissing committed Jan 4, 2016
1 parent cde7cfe commit 3b8484a
Show file tree
Hide file tree
Showing 8 changed files with 14 additions and 31 deletions.
5 changes: 5 additions & 0 deletions ChangeLog
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
v1.0.18
--------------------------------------------------------------------------------
* fixed race condition in connnection shutdown that could cause indefinite
hang, fixed cleanup of http2 worker threads, thanks to Yann Ylavic

v1.0.17
--------------------------------------------------------------------------------
* H2Timeout/H2KeepAliveTimeout now defaults to what ever is set for HTTP/1
Expand Down
2 changes: 1 addition & 1 deletion configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#

AC_PREREQ([2.69])
AC_INIT([mod_http2], [1.0.17], [[email protected]])
AC_INIT([mod_http2], [1.0.18], [[email protected]])

LT_PREREQ([2.2.6])
LT_INIT()
Expand Down
4 changes: 0 additions & 4 deletions mod-h2.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@
B25574941BEB6EFC0058F97B /* h2_task_output.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = h2_task_output.h; sourceTree = "<group>"; };
B25574951BEB6EFC0058F97B /* h2_task_queue.c */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.c; path = h2_task_queue.c; sourceTree = "<group>"; };
B25574961BEB6EFC0058F97B /* h2_task_queue.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = h2_task_queue.h; sourceTree = "<group>"; };
B25574971BEB6EFC0058F97B /* h2_to_h1.c */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.c; path = h2_to_h1.c; sourceTree = "<group>"; };
B25574981BEB6EFC0058F97B /* h2_to_h1.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = h2_to_h1.h; sourceTree = "<group>"; };
B25574991BEB6EFC0058F97B /* h2_util.c */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.c; path = h2_util.c; sourceTree = "<group>"; };
B255749A1BEB6EFC0058F97B /* h2_util.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = h2_util.h; sourceTree = "<group>"; };
B255749C1BEB6EFC0058F97B /* h2_version.h.in */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = h2_version.h.in; sourceTree = "<group>"; };
Expand Down Expand Up @@ -159,8 +157,6 @@
B25574961BEB6EFC0058F97B /* h2_task_queue.h */,
B255748F1BEB6EFC0058F97B /* h2_task.c */,
B25574901BEB6EFC0058F97B /* h2_task.h */,
B25574971BEB6EFC0058F97B /* h2_to_h1.c */,
B25574981BEB6EFC0058F97B /* h2_to_h1.h */,
B25574991BEB6EFC0058F97B /* h2_util.c */,
B255749A1BEB6EFC0058F97B /* h2_util.h */,
B2AB9ABB1C2ADBE100908DD6 /* h2_version.h */,
Expand Down
1 change: 1 addition & 0 deletions mod_http2/h2_io.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include <assert.h>

#include <apr_pools.h>
#include <apr_thread_mutex.h>
#include <apr_thread_cond.h>

Expand Down
9 changes: 0 additions & 9 deletions mod_http2/h2_mplx.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -705,9 +705,6 @@ apr_status_t h2_mplx_out_write(h2_mplx *m, int stream_id,
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;
Expand Down Expand Up @@ -744,12 +741,6 @@ apr_status_t h2_mplx_out_close(h2_mplx *m, int stream_id, apr_table_t *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;
}
}
else {
status = APR_ECONNABORTED;
Expand Down
4 changes: 2 additions & 2 deletions mod_http2/h2_version.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@
* @macro
* Version number of the http2 module as c string
*/
#define MOD_HTTP2_VERSION "1.0.17"
#define MOD_HTTP2_VERSION "1.0.18"

/**
* @macro
* Numerical representation of the version number of the http2 module
* release. This is a 24 bit number with 8 bits for major number, 8 bits
* for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
*/
#define MOD_HTTP2_VERSION_NUM 0x010011
#define MOD_HTTP2_VERSION_NUM 0x010012


#endif /* mod_h2_h2_version_h */
19 changes: 5 additions & 14 deletions mod_http2/h2_worker.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,19 +101,6 @@ static void* APR_THREAD_FUNC execute(apr_thread_t *thread, void *wctx)
return NULL;
}

static apr_status_t cleanup_join_thread(void *ctx)
{
h2_worker *w = ctx;
/* do the join only when the worker is aborted. Otherwise,
* we are probably in a process shutdown.
*/
if (w->thread && w->aborted) {
apr_status_t rv;
apr_thread_join(&rv, w->thread);
}
return APR_SUCCESS;
}

h2_worker *h2_worker_create(int id,
apr_pool_t *parent_pool,
apr_threadattr_t *attr,
Expand Down Expand Up @@ -147,7 +134,6 @@ h2_worker *h2_worker_create(int id,
return NULL;
}

apr_pool_pre_cleanup_register(w->pool, w, cleanup_join_thread);
apr_pool_create(&w->task_pool, w->pool);
apr_thread_create(&w->thread, attr, execute, w, w->pool);
}
Expand All @@ -156,6 +142,11 @@ h2_worker *h2_worker_create(int id,

apr_status_t h2_worker_destroy(h2_worker *worker)
{
if (worker->thread) {
apr_status_t status;
apr_thread_join(&status, worker->thread);
worker->thread = NULL;
}
if (worker->io) {
apr_thread_cond_destroy(worker->io);
worker->io = NULL;
Expand Down
1 change: 0 additions & 1 deletion mod_http2/h2_workers.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ h2_workers *h2_workers_create(server_rec *s, apr_pool_t *server_pool,
apr_atomic_set32(&workers->max_idle_secs, 10);

apr_threadattr_create(&workers->thread_attr, workers->pool);
apr_threadattr_detach_set(workers->thread_attr, 1);
if (ap_thread_stacksize != 0) {
apr_threadattr_stacksize_set(workers->thread_attr,
ap_thread_stacksize);
Expand Down

2 comments on commit 3b8484a

@lkraav
Copy link

@lkraav lkraav commented on 3b8484a Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up from f608b6b#commitcomment-15245605

dot18 still has the same issue, where browsers occasionally can't make connections if navigations buttons are pressed shortly after page has finished loading. I'm not sure about quantifying it, whether it's worse or better than dot16 was.

Weird new problem seen was that one of the apps behind mod_proxy started producing 502 errors for certain type of content. I'm not sure what is in there, because I can't see those pages now, but everything in this app definitely used to work before.

EDIT ^^^^ duh of course it's because Timeout 5 doesn't the allow the proxied app to serve some of its contents quickly enough!

Configuration used today:

Timeout 5
KeepAliveTimeout 20

Next up: testing

Timeout 10
KeepAliveTimeout 30
H2KeepAliveTimeout 60

@lkraav
Copy link

@lkraav lkraav commented on 3b8484a Jan 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over two days now, I would say things overall are better with H2KeepAliveTimeout 60 combined with the others. Occasionally though, I've seen browsers trying to make unsuccessful connections before loading a page or during loading assets from the page. Hitting reload doesn't help in that case. But after some undetermined amount of time passes, things resume to working normally again. I haven't diagnosed in detail yet whether it's exactly this 60 seconds that needs to be waited out in this case.

We don't really have a reliably working setup here yet, though, so hopefully additional tweaks can be thought of for testing.

Please sign in to comment.