disconnect / apache-websocket

Apache WebSocket module
Apache License 2.0
196 stars 46 forks source link

Make output bucket brigades thread safe #20

Open abligh opened 12 years ago

abligh commented 12 years ago

Please pull commit 04b57a6376cd9740db66e1394f1aef8696f6b4f0 from my fork. This fixes a bug whereby the output bucket brigade is created using the same pool and bucket allocator as the input thread is using, which causes a (very occasional) SEGV.

Diff is at: https://github.com/abligh/apache-websocket/commit/04b57a6376cd9740db66e1394f1aef8696f6b4f0

abligh commented 12 years ago

Please also pull this one: https://github.com/abligh/apache-websocket/commit/2d824f989aac196f42ad5127a290df04720bc2da

It would appear to need its own allocator as well as its own bucketbrigade allocator.

disconnect commented 12 years ago

I modified the code according to your first pull-request, but in a way that compiles under MSVC. As for the other "improve thread protection" changes, would you explain why you need another allocator and an allocator mutex?

abligh commented 12 years ago

Sure: You need another allocator (and pool) because the allocators themselves are not thread safe. Essentially they maintain a linked list of used and free memory, and there is no protection on the modification of that list. The default action on creating a pool is that the new pool inherits the allocator of its parent. This problematic when creating a new pool for a thread. The bucket brigade allocator is not really a new allocator (in the sense of a new thread safe allocator) because it gets its memory from the pool allocator attached to the pool you pass in. So to make allocation in the bucket brigade thread safe (which is what we need as the output bucket brigade is run in the created thread) we need to create a new pool with a different pool allocator (hence apr_allocator_create), and in that pool create a new bucket brigade allocator (apr_bucket_alloc_create).

I am not entirely sure why you need another allocator mutex, but the bug takes four or five hours to reproduce here, and that is what the apache-dev mailing list advised doing. For the sake of one line, and one call at set up time only, I put this in too.

Note the other change was to make the main thread mutex non-nested (rather than DEFAULT), as the default type is not always UNNESTED. Please see the warning here on apr_thread_mutex_create: http://apr.apache.org/docs/apr/0.9/group__apr__thread__mutex.html#g927e99580a669f577fbcb6508814ff12

Alex Bligh