Re: mod_http2 and Frequent wake-ups for mpm_event

[Yann Ylavic]

Hi Stefan,

On Sun, Feb 5, 2017 at 7:51 PM, Stefan Priebe - Profihost AG
 wrote:
>
> tested your patch against mod_ssl. I haven't seen any pool crashes again
> so it seems to fix this issue.
>
> But two new ones:

Possibly a promising fix suggested (in another thread) by yet another
talented Stefan :)
He has spotted a possible issue in mpm_event which could affect mainly
mod_http2.
I applied his suggestion in the attached patch, and did some
extrapolation for similar code in mod_h2 (so all possible errors are
mine...).
Would you test this one please?

@icing: I changed the parent pool of the slave connection from the
mplx pool to the master connection's (ptrans), just to simplify the
allocators in place for now.
I don't see it was needed from a concurrency POV, but if you wanted to
avoid locking when creating slaves' pool we can probably keep the
dedicated allocator finally (to reduce possible contention on
ptrans->mutex? No mutex needed I think since mplx doesn't seem to have
other subpools. Trade off with memory consumption, though).


Regards,
Yann.
Index: server/mpm/event/event.c
===
--- server/mpm/event/event.c	(revision 1781789)
+++ server/mpm/event/event.c	(working copy)
@@ -1749,6 +1749,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
 if (ptrans == NULL) {
 /* create a new transaction pool for each accepted socket */
 apr_allocator_t *allocator;
+apr_thread_mutex_t *mutex = NULL;
 
 apr_allocator_create();
 apr_allocator_max_free_set(allocator,
@@ -1762,6 +1763,8 @@ static void * APR_THREAD_FUNC listener_thread(apr_
 signal_threads(ST_GRACEFUL);
 return NULL;
 }
+apr_thread_mutex_create(, APR_THREAD_MUTEX_DEFAULT, ptrans);
+apr_allocator_mutex_set(allocator, mutex);
 }
 apr_pool_tag(ptrans, "transaction");
 
Index: modules/http2/h2_conn.c
===
--- modules/http2/h2_conn.c	(revision 1781789)
+++ modules/http2/h2_conn.c	(working copy)
@@ -247,9 +247,10 @@ apr_status_t h2_conn_pre_close(struct h2_ctx *ctx,
 return status;
 }
 
-conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent)
+conn_rec *h2_slave_create(conn_rec *master, int slave_id)
 {
-apr_allocator_t *allocator;
+apr_allocator_t *allocator = NULL;
+apr_thread_mutex_t *mutex = NULL;
 apr_pool_t *pool;
 conn_rec *c;
 void *cfg;
@@ -264,9 +265,11 @@ apr_status_t h2_conn_pre_close(struct h2_ctx *ctx,
  * another thread.
  */
 apr_allocator_create();
-apr_pool_create_ex(, parent, NULL, allocator);
+apr_pool_create_ex(, master->pool, NULL, allocator);
 apr_pool_tag(pool, "h2_slave_conn");
 apr_allocator_owner_set(allocator, pool);
+apr_thread_mutex_create(, APR_THREAD_MUTEX_DEFAULT, pool);
+apr_allocator_mutex_set(allocator, mutex);
 
 c = (conn_rec *) apr_palloc(pool, sizeof(conn_rec));
 if (c == NULL) {
Index: modules/http2/h2_conn.h
===
--- modules/http2/h2_conn.h	(revision 1781789)
+++ modules/http2/h2_conn.h	(working copy)
@@ -66,7 +66,7 @@ typedef enum {
 h2_mpm_type_t h2_conn_mpm_type(void);
 
 
-conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent);
+conn_rec *h2_slave_create(conn_rec *master, int slave_id);
 void h2_slave_destroy(conn_rec *slave);
 
 apr_status_t h2_slave_run_pre_connection(conn_rec *slave, apr_socket_t *csd);
Index: modules/http2/h2_mplx.c
===
--- modules/http2/h2_mplx.c	(revision 1781789)
+++ modules/http2/h2_mplx.c	(working copy)
@@ -33,6 +33,7 @@
 #include "h2_private.h"
 #include "h2_bucket_beam.h"
 #include "h2_config.h"
+#include "h2_session.h"
 #include "h2_conn.h"
 #include "h2_ctx.h"
 #include "h2_h2.h"
@@ -245,7 +246,7 @@ static void h2_mplx_destroy(h2_mplx *m)
   m->id, (int)h2_ihash_count(m->tasks));
 check_tx_free(m);
 /* pool will be destroyed as child of h2_session->pool,
-   slave connection pools are children of m->pool */
+   slave connection pools are children of m->c->pool */
 }
 
 /**
@@ -259,32 +260,27 @@ static void h2_mplx_destroy(h2_mplx *m)
  *   their HTTP/1 cousins, the separate allocator seems to work better
  *   than protecting a shared h2_session one with an own lock.
  */
-h2_mplx *h2_mplx_create(conn_rec *c, apr_pool_t *parent, 
-const h2_config *conf, 
-apr_interval_time_t stream_timeout,
-h2_workers *workers)
+h2_mplx *h2_mplx_create(h2_session *session, h2_workers *workers)
 {
 

Re: Is mpm_event missing allocator mutexes?

[Yann Ylavic]

Hi Stefan,

On Sun, Feb 5, 2017 at 10:39 PM, Stefan Fritsch  wrote:
>
> apr_allocator uses a mutex to be thread safe. Pools use this mutex also to
> protect sub-pool creation, cleanup registering, etc. When apr creates the
> initial allocator and global_pool in apr_pool_initialize(), it also creates a
> mutex for this allocator. But mpm_event creates a new allocator for each
> transaction pool and does not create a mutex for it. And gdb shows that in
> listener_thread() at the get_worker() call, ptrans->allocator->mutex is NULL
> (checked in 2.4.25).

Howesome catch!

>
> Shouldn't we allocate a mutex there and call apr_allocator_mutex_set()? If no,
> why not. If yes, why does it still work as well as it does right now? Or could
> the lacking mutex explain some weird segfaults?

I think it always worked with HTTP/1 because we never race on request
create/destroy (i.e. on creation/destruction of ptrans' subpools) nor,
AFAICT, with subpools of the request (usually used by the same
thread).
We do have concurrent threads with subpools but they are created from
pconf/pchild (hence mutexed).

With HTTP/2 things get complicated because there are multiple subpools
(of subpools) of ptrans (slave connections, mplx, ...) concurrently
created/destroyed, and that obviously can't work without a mutex.

Maybe since 2.4.25, and the changes wrt kept alive connections early
closing on graceful (or/and the mpm event wakeup patch, not sure
whether s.priebe@ finally runs mpm_event w/ or w/o it applied), we get
more concurrency with ptrans being destroyed "out of band" (while h2
still has ongoing work).

Anyway, it also seems to me that we should make ptrans mutexed, and so
do we for all allocators dedicated to pools created in mod_http2.


Regards,
Yann.

Is mpm_event missing allocator mutexes?

[Stefan Fritsch]

Hi,

I may be missing something but this looks wrong to me:

apr_allocator uses a mutex to be thread safe. Pools use this mutex also to 
protect sub-pool creation, cleanup registering, etc. When apr creates the 
initial allocator and global_pool in apr_pool_initialize(), it also creates a 
mutex for this allocator. But mpm_event creates a new allocator for each 
transaction pool and does not create a mutex for it. And gdb shows that in 
listener_thread() at the get_worker() call, ptrans->allocator->mutex is NULL 
(checked in 2.4.25).

Shouldn't we allocate a mutex there and call apr_allocator_mutex_set()? If no, 
why not. If yes, why does it still work as well as it does right now? Or could 
the lacking mutex explain some weird segfaults?

Cheers,
Stefan

Re: mod_http2 and Frequent wake-ups for mpm_event

[Stefan Priebe - Profihost AG]

Hi Yann,

tested your patch against mod_ssl. I haven't seen any pool crashes again
so it seems to fix this issue.

But two new ones:

Program terminated with signal SIGABRT, Aborted.
#0  0x7ff523558067 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#0  0x7ff523558067 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x7ff523559448 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x7ff5235961b4 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x7ff52359b98e in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#4  0x7ff52359c923 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#5  0x7ff523b06c83 in apr_allocator_destroy (allocator=0x7ff4de50)
at memory/unix/apr_pools.c:152
#6  0x556e7986bbef in task_destroy (m=0x7ff46402a028,
task=0x7ff4d0029f00,
called_from_master=0) at h2_mplx.c:400
#7  0x556e7986ce5b in task_done_iter (ctx=,
val=) at h2_mplx.c:1064
#8  0x7ff523afe5e6 in apr_hash_do (
comp=comp@entry=0x556e7987f180 ,
rec=rec@entry=0x7ff5037e5480,
ht=) at tables/apr_hash.c:542
#9  0x556e7987fb5f in h2_ihash_iter (ih=,
fn=fn@entry=0x556e7986ce50 ,
ctx=ctx@entry=0x7ff46402a028)
at h2_util.c:315
#10 0x556e7986d463 in h2_mplx_release_and_join (m=0x7ff46402a028,
wait=0x7ff464029fd0) at h2_mplx.c:619
#11 0x556e79872ae4 in session_pool_cleanup (data=0x7ff464020318)
at h2_session.c:827
#12 0x7ff523b0748e in run_cleanups (cref=0x7ff464029fa8)
at memory/unix/apr_pools.c:2352
#13 apr_pool_destroy (pool=0x7ff464029f38) at memory/unix/apr_pools.c:804
#14 0x7ff523b07745 in apr_pool_clear (pool=0x7ff4fc0150e8)
at memory/unix/apr_pools.c:769
#15 0x556e798aa698 in ap_push_pool (queue_info=0x1e2e,
pool_to_recycle=0x1ebb) at fdqueue.c:234
#16 0x556e798a59da in process_lingering_close (cs=0x7ff4fc015378,
pfd=0x556e7b8bd888) at event.c:1513
#17 0x556e798a9510 in listener_thread (thd=0x1e2e,
dummy=0x547b44ea3e1b3)
at event.c:1837
#18 0x7ff5238d60a4 in start_thread ()
   from /lib/x86_64-linux-gnu/libpthread.so.0
#19 0x7ff52360b62d in clone () from /lib/x86_64-linux-gnu/libc.so.6

and

Program terminated with signal SIGSEGV, Segmentation fault.
#0  apr_bucket_alloc (size=size@entry=64,
list=list@entry=0x3fb50d4a7aeb1d49)
at buckets/apr_buckets_alloc.c:128
#0  apr_bucket_alloc (size=size@entry=64,
list=list@entry=0x3fb50d4a7aeb1d49)
at buckets/apr_buckets_alloc.c:128
#1  0x7ff523d2b1d3 in apr_bucket_heap_create (
buf=0x7ff51003b3a8 "
\311\021A\216y\034\347\034Wy\360\343R\275\226o\020iw\227r\337\377",
length=1300, free_func=0x7ff523d2ab50 ,
list=0x3fb50d4a7aeb1d49) at buckets/apr_buckets_heap.c:81
#2  0x556e79884f85 in append_scratch (io=0x7ff4440377c8)
at h2_conn_io.c:165
#3  0x556e79884ffa in assure_scratch_space (io=0x7ff4440377c8)
at h2_conn_io.c:182
#4  0x556e79885ce8 in h2_conn_io_pass (io=io@entry=0x7ff4440377c8,
bb=0x7ff444133698) at h2_conn_io.c:393
#5  0x556e798732be in on_send_data_cb (ngh2=,
frame=, framehd=, length=1291,
source=, userp=0x7ff444037780) at h2_session.c:648
#6  0x7ff5243dde95 in ?? () from
/usr/lib/x86_64-linux-gnu/libnghttp2.so.14
#7  0x7ff5243deea9 in nghttp2_session_send ()
   from /usr/lib/x86_64-linux-gnu/libnghttp2.so.14
#8  0x556e79875857 in h2_session_send (session=0x7ff444037780)
at h2_session.c:1376
#9  0x556e79878b6c in h2_session_process (session=0x7ff444037780,
async=2062228809) at h2_session.c:2208
#10 0x556e79867788 in h2_conn_run (ctx=0x7ff4440376b0, c=0x7ff51003b6a8)
at h2_conn.c:214
#11 0x556e7986a421 in h2_h2_process_conn (c=0x7ff51003b6a8) at
h2_h2.c:658
#12 0x556e7980d050 in ap_run_process_connection (c=0x7ff51003b6a8)
at connection.c:42
#13 0x556e798a7590 in process_socket (my_thread_num=,
my_child_num=, cs=0x7ff51003b618, sock=,
p=, thd=) at event.c:1134
#14 worker_thread (thd=0x40, dummy=0x3fb50d4a7aeb1d49) at event.c:2137
#15 0x7ff5238d60a4 in start_thread ()
   from /lib/x86_64-linux-gnu/libpthread.so.0
#16 0x7ff52360b62d in clone () from /lib/x86_64-linux-gnu/libc.so.6

Greets,
Stefan

Am 02.02.2017 um 11:09 schrieb Yann Ylavic:
> Hi Stefan,
> 
> On Tue, Jan 31, 2017 at 4:01 PM, Stefan Priebe - Profihost AG
>  wrote:
>>
>> any ideas?
> 
> I wonder if the attached patch (related to mod_ssl and proposed for
> another segfault report) could help in your case.
> 
> Would you mind give it a try?
> 
> 
> Thanks,
> Yann.
> 

Re: Autobuild Progress (was Re: Automated tests)

[Daniel Ruggeri]

On 1/31/2017 4:30 PM, Jacob Champion wrote:
> On 01/30/2017 05:39 PM, Daniel Ruggeri wrote:
>> I'm tremendously inspired by this work. What are your thoughts on the
>> idea of having a series of docker container builds that compile and run
>> the test suite on various distributions? I'll volunteer to give this a
>> whack since it's something that's been in the back of my mind for a long
>> while...
>
> I think that would be awesome. The cheaper we can make new test
> distributions, the easier we can test all sorts of different
> configurations (which, given how many knobs and buttons we expose, is
> important).
>
> I don't know how much of Infra's current Puppet/Buildbot framework is
> Docker-friendly, but if there's currently no cheap virtualization
> solution there for build slaves, then anything we added would
> potentially be useful for other ASF projects as well. Definitely
> something to start a conversation over.
>

Yes, definitely. Thinking more about this, even adding something
heavyweight like a type 2 hypervisor could potentially provide value so
long as the VM image is stripped down enough and we don't leave junk
behind on the slave. I'm not concerned about Puppet and buildbot
integration since Puppet is a great way to manage the configuration of
the slave (assuming that's what it's used for) which makes it easy to
have docker, virtualbox, vagrant or whatever installed and configured.

As far as buildbot, I'm sure it will support the execution of a script
which is all that's needed. My latest work with the
RemoteIPProxyProtocol stuff has me compiling httpd on my build machine
and standing up a docker container with haproxy inside. Hitting the
resulting build under various circumstances with wget scratches the
itch. I've got this distilled down into only four files (Dockerfile,
haproxy.cfg, setup script and test script). This is nice because...
well... I just don't want to install haproxy on my build box for this

In any event, I've started the conversation with builds@a.o to see
what's doable. Can crosspost or just return with feedback when I hear.


> (Side thought: since raw speed is typically one of the top priorities
> for a CI test platform, we'd have to carefully consider which items we
> tested by spinning up containers and which we ran directly on a
> physical machine. Though I don't know how fast Docker has gotten with
> all of the fancy virtualization improvements.)

Amen to that. Docker's quite fast since lxc and all the stuff around it
are very lightweight. The slowest parts are pulling the base image and
setting it up (installing compilers, the test framework, tools, etc).
This can be sped up greatly by building the image and publishing it back
to a (or "the") registry or keeping it local on the machine, but we'd
then have to maintain images which I'm not a fan of.


>
>> I think with the work you've done and plan to do, a step like above to
>> increase our ability to test against many distributions all at once (and
>> cheaply) and also making the test framework more approachable, we could
>> seriously increase our confidence when pulling the trigger on a release
>> or accepting a backport.
>
> +1. It'll take some doing (mostly focused on the coverage of the test
> suite itself), but we can get there.
>
>> I'm also a big fan of backports requiring tests, but am honestly
>> intimidated by the testing framework...
>
> What would make it less intimidating for you? (I agree with you, but
> I'm hoping to get your take without biasing it with my already-strong
> opinions. :D)

Opinions here... so take them with a grain of salt.
* The immediate barrier to entry is doco. From the test landing page,
you are greeted with a list of what's in the test project and links to
the components. Of the links there (our super important one is Perl
Framework), only the flood link leads to a useful getting started guide.
This may be lazy and kinda preachy, but not having good developer info
easily accessible is a Bad Thing(tm) since it's a surefire way to scare
off those potentially interested in participating in a project.
* It's also intimidating when a developer realizes they need to learn a
new skill set to create tests. Writing tests for Perl's testing
framework feels archaic, and I'm not sure is a skill many potential
contributors would possess unless they have developed Perl modules for
distribution. I understand the history of the suite so I _get_ why it's
this way... it's just that it is likely a turn-off. Disclaimer: I'm not
saying Perl has a bad testing framework. I have yet to find a testing
framework I'm a big fan of since they all have their idiosyncrasies. No
holy wars, please :-)
* Another barrier that I think is very much worth pointing out is that
several Perl modules must be installed. I have some history fighting
with Crypt::SSLeay to do what I want because it can be rather finicky.
For example, if your system libssl is something ancient like 0.9.8 but
you compiled httpd with 1.0.2, 

Re: Summary: Broken FastCGI with httpd

[Jim Jagielski]

> On Jan 30, 2017, at 4:21 PM, Jacob Champion  wrote:
> 
> On 01/30/2017 01:17 PM, Jim Jagielski wrote:
>> Looking over fpm_main, Apache is detected iff PHP sees the proxy:balancer 
>> and/or
>> proxy:fcgi prefix. Looking at the logic paths related to 'apache_was_here'
> 
> No, apache_was_here is only *new* Apache. There are other fixups that trigger 
> on other variables; search for "apache specific" for another example.
> 

Not seeing anything fpm_main.c...

Re: Openssl 1.1.x compatibility

[Luca Toscano]

2017-01-27 15:38 GMT+01:00 Luca Toscano :

> Hi everybody,
>
> once in a while on users@ it is asked if httpd 2.4.x supports Openssl
> 1.1.x, but afaik http://svn.apache.org/viewvc/httpd/httpd/branches/2.
> 4.x-openssl-1.1.0-compat is still work in progress. Any plans to release
> this work during the next months? It would be really great, but I am
> completely ignorant about how feasible it would be and if we hit road
> blockers that stopped the development.
>


Gentle ping to the list :)

Luca