Joe Orton wrote:
Some quick review in-line below. Where do you envisage this code going,
apr-util or apr? I think the former is more appropriate since the code
is entirely platform-independent; it needs a build system patch too
either way :). The code style is good apart from the use of the space
after * in pointer arguments; should be "apr_pool_t *pool" not
"apr_pool_t * pool".
The patch is for apr-util. But I have no preference. The build system
for autotools works as is. Windows is another story. :-)
That's a result of using GNU indent as suggested at
http://httpd.apache.org/dev/styleguide.html.
indent -i4 -npsl -di0 -br -nce -d0 -cli0 -npcs -nfc1 -nut
On Tue, May 02, 2006 at 10:46:19AM -0700, Henry Jen wrote:
+#ifdef __cplusplus
+extern "C"
+{
+#if 0
+};
+#endif
This is weird, what's it for? To un-confuse an editor's indenting logic
or something?
Un-confuse indent, yes.
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct _apr_thread_pool apr_thread_pool_t;
Don't begin type or variable names with an underscore (such things are
reserved by the C library).
Right, I need to fix this.
I misunderstood it as reserved to be be used by "implementation" of
"library". ;-)
...
+APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
+ apr_pool_t * pool,
It is conventional to have the pool as the last argument (or first in
rare cases).
Thanks for pointing out. I happen to refer to prototype with 2
parameters and apr_array_make. :-)
+struct _apr_thread_pool
+{
+ apr_pool_t *parent;
that field doesn't appear to be used?
Removed in latest patch.
+
+/*
+ * NOTE: This function is not thread safe by itself. Caller should hold the
lock
+ */
+apr_thread_pool_task_t *apr_thread_pool_tasks_pop(apr_thread_pool_t * me)
is this function supposed to be public or private? It should be static
if the latter, and shouldn't have an apr_ prefix so it's easy for the
reader to distinguish.
Will fix.
+ APR_RING_ELEM_INIT(elt, link);
+ elt->thd = t;
+ elt->stop = 0;
+ apr_thread_mutex_lock(me->lock);
+ ++me->idle_cnt;
+ APR_RING_INSERT_TAIL(me->idle_thds, elt, _apr_thread_list_elt, link);
+ apr_thread_mutex_unlock(me->lock);
+
+ apr_thread_mutex_lock(me->cond_lock);
+ apr_thread_cond_wait(me->cond, me->cond_lock);
+ apr_thread_mutex_unlock(me->cond_lock);
+
+ apr_thread_mutex_lock(me->lock);
Lots of missing error checking for mutex/condvar calls throughout this
code.
Hmm, what do you suggest to do?
I don't expect error to occurs unless something seriously screwed up.
Those are blocking call, no timeout is specified.
+{
+ apr_thread_pool_t *_self = me;
+
+ _self->terminated = 1;
+ apr_thread_pool_idle_max_set(_self, 0);
+ while (_self->busy_cnt) {
+ apr_sleep(20 * 1000); /* spin lock with 20 ms */
+ }
I don't think that's strictly safe, it should be using the atomic
functions if it must be done without the mutex held.
It is not intent to. When set the terminated to 1, the count shouldn't
increase, only decrease. A quick snapshot works well.
...
+APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
+ apr_pool_t * pool,
+ apr_size_t init_threads,
+ apr_size_t max_threads)
+{
+ apr_thread_t *t;
+ apr_status_t rv = APR_SUCCESS;
+ apr_pool_t *p;
+
+ if (!me) {
+ return APR_BADARG;
+ }
Don't do argument validation, segfaulting if the caller screws up is
standard practice.
In this case, agree. :-)
+ *me = (apr_thread_pool_t *) apr_pcalloc(pool, sizeof(**me));
+ if (!*me) {
+ return APR_ENOMEM;
+ }
... or OOM checking again.
I don't understand what do you suggest to do.
...
+static apr_thread_pool_task_t *task_new(apr_thread_pool_t * me,
+ apr_thread_start_t func,
+ void *param, apr_byte_t priority)
+{
+ apr_thread_pool_task_t *t;
+
+ if (APR_RING_EMPTY(me->recycled_tasks, _apr_thread_pool_task, link)) {
+ t = apr_pcalloc(me->pool, sizeof(*t));
+ if (NULL == t) {
+ return NULL;
+ }
No need to check for NULL from apr_p*alloc again.
So you are suggesting just let the app segfault when out of memory?
Cheers,
Henry
