Re: APR threadpool and memory pool weirdness

[Henry Jen]

Try to add a sleep call after push a couple tasks and see how that works.

Cheers,
Henry

2011/7/27 Cosmin Luță cosmin.l...@avira.com:
 Hi all,

 I have a rather strange use case for APR which leads to quick memory
 increase on Linux (at least): a program creates (in a loop) memory
 pools which are passed to tasks in a threadpool. Each task will call
 apr_pool_destroy on its pool (received as an argument).

 You can see the demo code here:

 http://pastebin.com/KWPKsLrc

 You can try to compile with:

 gcc -g -O0 -o test_pools  `apr-1-config --cflags --includes
 --cppflags`  -lapr-1 -laprutil-1 test_pools.c

 This is what I compile against (on an OpenSUSE 11.4 machine):

 % rpm -qa | grep libapr
 libapr-util1-debuginfo-1.3.9-9.2.x86_64
 libapr1-devel-1.4.2-3.1.x86_64
 libapr-util1-devel-1.3.9-9.2.x86_64
 libapr-util1-1.3.9-9.2.x86_64
 libapr1-1.4.2-3.1.x86_64
 libapr1-debuginfo-1.4.2-3.1.x86_64


 So, can anyone tell me what I'm missing here? I'm aware that it's not
 very usual to create a pool in a thread, then pass it as a pointer to
 another, but I don't think that's the source of the memory increase.
 Any ideas what I can do to stop this from happening?


 Best regards,


 --
 --
 Cosmin Luță
 Software Developer
 AVIRA Soft srl

Re: [Vote] apr-util 1.5.x - trunk

[Henry Jen]

2010/10/7 Graham Leggett minf...@sharp.fm:
 On 07 Oct 2010, at 12:22 PM, Jeff Trawick wrote:

 These choices seem skewed to me.  apr is apr-util/trunk is a
 different concept than rename 1.5.x to trunk.  Conceptually, apr is
 apr-util trunk whatever we decide.

 I disagree, in the past, we had two projects, each with an independent trunk
 and release cycle, one called apr, the other called apr-util. We have chosen
 to retire the apr-util project, and have copied the functionality into apr,
 but that doesn't make the apr-util project go away.

 We will still need to make releases on apr-util in the v1.x series, and we
 may need to bump v1.3 to v1.4, etc. For this, we need a properly functional
 trunk, otherwise those following the standard svn conventions face problems.


To me, I only care two things,

1. trunk should be where the latest development is going on.
2. avoid to have two different development tree(fork)

Seems to me, apr-util is merged into apr project and should have the
same release cycle. apr-util should only be maintained for sustaining
old releases but not new ones.

My understanding could be wrong, this is why we need clarification for
people like me.

Cheers,
Henry

Re: apr-util 1.5.x - trunk

[Henry Jen]

2010/10/5 Nick Kew n...@apache.org:
 On Tue, 05 Oct 2010 09:33:04 -0500
 William A. Rowe Jr. wr...@rowe-clan.net wrote:

 On 10/5/2010 2:40 AM, Joe Orton wrote:
  Any objection to renaming the apr-util 1.5.x branch to trunk?  It is
  the trunk for that tree now.

 -.5, because for the confusion it saves the dozen of us, many more dozens
 will be confused by checking out apr and apr-util trunks as they have in
 the past, only to be confronted by a checkout that doesn't work.

 Good point, well made.  We're software developers, not coppicers.
 How many trunks do we need?

 But it does perhaps highlight a need to be clearer about where we are.
 Might another idea be to have an apr-util/trunk/ containing nothing
 but a README explaining the situation?


+1.

Cheers,
Henry

Re: [PATCH] PR 49709 apr_thread_pool race condition caused by use of separate mutexes?

[Henry Jen]

2010/9/27 Nick Kew n...@apache.org:

 On 27 Sep 2010, at 13:12, Jeff Trawick wrote:

 See https://issues.apache.org/bugzilla/show_bug.cgi?id=49709 and attached 
 patch.

 The patch makes conceptual sense to me, though I haven't studied the 
 details.  Perhaps someone has a head start on learning the apr thread pool 
 implementation?

The patch looks good to me. As cond_wait actually release the mutex,
it should work just fine.


 The code in question was contributed by Henry Jen, and committed by wrowe in 
 r492362.
 Henry has made some good contributions, but I'm not sure if he participates 
 here or just
 contributes from a distance.

 Henry, do you follow this list?  If no reply, maybe you (Jeff) should copy 
 him your post.


Yes I am, but not paying to much attention nowadays.

Cheers,
Henry

Re: [PATCH] PR 49709 apr_thread_pool race condition caused by use of separate mutexes?

[Henry Jen]

OK, I tried this on my OpenSolaris box, and believe I figured out what
is going on.

The dead-lock happens when the initial one worker thread get into idle
first as no tasks are pushed yet, then two push comes in. Under this
circumstance, there is one thread idle, thus no new thread will be
created.

Now the idle thread get to pick up the first task, but is blocked,
thus no other thread will pick up the second task, which caused the
dead-lock.

As explained, this scenario is not supported by design as we expect
the task should not be blocking. Understood the cooperative approach
is not optimal, but that is all we needed at that moment and simplify
things a lot.

Cheers,
Henry

2010/9/27 Henry Jen henry...@ztune.net:
 2010/9/27 Jeff Trawick traw...@gmail.com:
 On Mon, Sep 27, 2010 at 4:55 PM, Henry Jen henry...@ztune.net wrote:
 2010/9/27 Nick Kew n...@apache.org:


 (The next one is
 https://issues.apache.org/bugzilla/show_bug.cgi?id=48722, which has no
 patch.  Maybe I can debug it.)


 I have a quick glance at it, and I would like to make a note that it
 is not recommended to have tasks that can block a thread. Apparently
 it can have dead lock if two blocking tasks blocks each other.

 The origin design for the thread pool is to have a set of threads can
 be used to execute a collections of tasks need to be done, each task
 can be finished in a fair amount of time. In other words, the task
 should not block the thread from execution for long and should never
 have external dependencies on other tasks to avoid dead-lock. If a
 task actually need some synchronization, it probably makes more sense
 to have its own thread for doing it.

 However, I think this particular test case should work if implement
 correctly. I wonder if the fact waiting and flag is not marked
 volatile cause any differences. I would like to test that, but on my
 Mac, when trying to build apr trunk, I got following error. How can I
 fix it? (Sorry that I haven't build apr for a long time...)

 sunfish:apr henryjen$ ./configure
 configure: error: cannot run /bin/sh build/config.sub

 Cheers,
 Henry

Re: Attn New Committers

[Henry Jen]

On Sat, Jun 14, 2008 at 1:42 PM, Ruediger Pluem [EMAIL PROTECTED] wrote:



 On 06/14/2008 10:20 PM, William A. Rowe, Jr. wrote:

 Please don't do this;

 ---
 Revision 663845 - Directory Listing
 Modified Fri Jun 6 07:39:52 2008 UTC (8 days, 12 hours ago) by henryjen

 backport r663342 from trunk
 ---

 Such commit messages aren't legible when comparing a branch to trunk,
 please
 repeat the purpose of the commit in the log message for the backport!


Thanks for the correction, I apologize for the mistake.



 There is an IMHO pretty cool script from Joe Orton that is very helpful
 for backporting trunk revisions. It creates a file with all the original
 commit messages that could be used as commit message for the backport
 plus it adds the numbers of the revisions that get backported. It
 can be found here:

 http://people.apache.org/~jorton/svn.mergehttp://people.apache.org/%7Ejorton/svn.merge


This is nice, thanks for the pointer.

Cheers,
Henry

Re: solaris atomic flaw

[Henry Jen]

Seems like an easy fix, the (void*) cast was put for the wrong parameter.

See http://docs.sun.com/app/docs/doc/816-5180/atomic-cas-ptr-9f?a=view
for the prototype for atomic_cas_ptr.

Trunk version add -Werror which caused build to fail on warning.

Attached is a one line patch for fix.

Cheers,
Henry


On Fri, May 30, 2008 at 10:16 PM, William A. Rowe, Jr.
[EMAIL PROTECTED] wrote:
 Henry Jen wrote:

 Trunk failed to compile with following error:

 atomic/unix/solaris.c: In function `apr_atomic_casptr':
 atomic/unix/solaris.c:71: warning: passing arg 2 of `atomic_cas_ptr'
 discards qualifiers from pointer target type
 make[1]: *** [atomic/unix/solaris.lo] Error 1
 make[1]: Leaving directory
 `/export/home/henryjen/prj/apache.org/apr-trunk'
 make: *** [all-recursive] Error 1

 Will investigate.

 I suspect this is the misplaced volatile modifier which needs to be fixed
 in 2.0, not before.

Index: atomic/unix/solaris.c
===
--- atomic/unix/solaris.c	(revision 661957)
+++ atomic/unix/solaris.c	(working copy)
@@ -68,7 +68,7 @@
 
 APR_DECLARE(void*) apr_atomic_casptr(volatile void **mem, void *with, const void *cmp)
 {
-return atomic_cas_ptr(mem, cmp, (void*) with);
+return atomic_cas_ptr(mem, (void*) cmp, with);
 }
 
 APR_DECLARE(void*) apr_atomic_xchgptr(volatile void **mem, void *with)

Re: [VOTE] Release apr-1.3.0, apr-util-1.3.0

[Henry Jen]

I got a different result from Bojan, trunk still fail on OpenSolaris 2008.05.

[EMAIL PROTECTED]:~/prj/apache.org/apr-trunk/test$ ./testall -v
testatomic  : SUCCESS
testdir : SUCCESS
testdso : SUCCESS
testdup : SUCCESS
testenv : SUCCESS
testfile: SUCCESS
testfilecopy: SUCCESS
testfileinfo: SUCCESS
testflock   : SUCCESS
testfmt : SUCCESS
testfnmatch : SUCCESS
testargs: SUCCESS
testhash: SUCCESS
testipsub   : SUCCESS
testlock: SUCCESS
testcond: SUCCESS
testlfs : SUCCESS
testmmap: SUCCESS
testnames   : SUCCESS
testoc  : SUCCESS
testpath: SUCCESS
testpipe: SUCCESS
testpoll: SUCCESS
testpools   : SUCCESS
testproc: SUCCESS
testprocmutex   : SUCCESS
testrand: SUCCESS
testsleep   : SUCCESS
testshm : -Line 254: Error destroying shared memory block
(22): Invalid argument

FAILED 1 of 6
testsock: |Line 270: Cannot test if connect completes synchronously
SUCCESS
testsockets : SUCCESS
testsockopt : SUCCESS
teststr : SUCCESS
teststrnatcmp   : SUCCESS
testtable   : SUCCESS
testtemp: SUCCESS
testthread  : SUCCESS
testtime: SUCCESS
testud  : SUCCESS
testuser: SUCCESS
testvsn : SUCCESS
Failed TestsTotal   FailFailed %
===
testshm 6  1 16.67%

testshm failure on OpenSolaris

[Henry Jen]

My guess is that shmctl(shmid, IPC_RMID, NULL) is called twice, and
the second time will cause EINVAL.

It is called once in apr_shm_remove(line 411), and will be called
again in shm_cleanup_owner(line 72) caused by apr_shm_destroy.

Comments, suggestions?

Cheers,
Henry

On Fri, May 30, 2008 at 11:31 PM, Henry Jen [EMAIL PROTECTED] wrote:
 I got a different result from Bojan, trunk still fail on OpenSolaris 2008.05.

 [EMAIL PROTECTED]:~/prj/apache.org/apr-trunk/test$ ./testall -v
 testatomic  : SUCCESS
 testdir : SUCCESS
 testdso : SUCCESS
 testdup : SUCCESS
 testenv : SUCCESS
 testfile: SUCCESS
 testfilecopy: SUCCESS
 testfileinfo: SUCCESS
 testflock   : SUCCESS
 testfmt : SUCCESS
 testfnmatch : SUCCESS
 testargs: SUCCESS
 testhash: SUCCESS
 testipsub   : SUCCESS
 testlock: SUCCESS
 testcond: SUCCESS
 testlfs : SUCCESS
 testmmap: SUCCESS
 testnames   : SUCCESS
 testoc  : SUCCESS
 testpath: SUCCESS
 testpipe: SUCCESS
 testpoll: SUCCESS
 testpools   : SUCCESS
 testproc: SUCCESS
 testprocmutex   : SUCCESS
 testrand: SUCCESS
 testsleep   : SUCCESS
 testshm : -Line 254: Error destroying shared memory block
 (22): Invalid argument

 FAILED 1 of 6
 testsock: |Line 270: Cannot test if connect completes 
 synchronously
 SUCCESS
 testsockets : SUCCESS
 testsockopt : SUCCESS
 teststr : SUCCESS
 teststrnatcmp   : SUCCESS
 testtable   : SUCCESS
 testtemp: SUCCESS
 testthread  : SUCCESS
 testtime: SUCCESS
 testud  : SUCCESS
 testuser: SUCCESS
 testvsn : SUCCESS
 Failed TestsTotal   FailFailed %
 ===
 testshm 6  1 16.67%

Re: [VOTE] Release apr-1.3.0, apr-util-1.3.0

[Henry Jen]

On Fri, May 30, 2008 at 6:04 PM, Lucian Adrian Grijincu
[EMAIL PROTECTED] wrote:
 Ubuntu 8.04 - 2.6.24-17-generic SMP

 +/-1
 [+1]  Release apr-1.3.0 as GA
 [+1]  Release apr-util-1.3.0 as GA


 On Sat, May 31, 2008 at 3:31 AM, Bojan Smojver
 --
 Failed Tests   Total Fail Failed %
 ===
 testshm61 16.67%
 Same on ubuntu.


Same on OpenSolaris 2008.05. 1.3.x branch failed the same way.
Trunk failed to compile with following error:

atomic/unix/solaris.c: In function `apr_atomic_casptr':
atomic/unix/solaris.c:71: warning: passing arg 2 of `atomic_cas_ptr'
discards qualifiers from pointer target type
make[1]: *** [atomic/unix/solaris.lo] Error 1
make[1]: Leaving directory `/export/home/henryjen/prj/apache.org/apr-trunk'
make: *** [all-recursive] Error 1

Will investigate.

Cheers,
Henry

Re: apr_reslist semantics

[Henry Jen]

On Fri, May 23, 2008 at 12:28 PM, Nick Kew [EMAIL PROTECTED] wrote:
 On Mon, 12 May 2008 20:22:09 +0100
 Nick Kew [EMAIL PROTECTED] wrote:

 In https://issues.apache.org/bugzilla/show_bug.cgi?id=42841 ,
 Tom points out an issue that gives problems with MySQL
 (and possibly other DBD drivers) and suggests that a change
 to apr_reslist semantics would fix it.  Tom also attaches
 a patch implementing his proposed change.

 [chop]

 After far too long, I've revisited this issue and Tom's patch.

 It seems to me we can get *both* semantics at once:

 * Keep reslist_maint as is, preserving its existing semantics
 * Check the idle time of a resource in apr_reslist_acquire,
  and kill a resource if it's too old.

 There are other considerations, but a minimal effective patch
 looks like:

 ===
 --- apr_reslist.c   (revision 659614)
 +++ apr_reslist.c   (working copy)
 @@ -292,13 +292,22 @@
  {
 apr_status_t rv;
 apr_res_t *res;
 +apr_time_t now = apr_time_now();

 apr_thread_mutex_lock(reslist-listlock);
 /* If there are idle resources on the available list, use
  * them right away. */
 -if (reslist-nidle  0) {
 +while (reslist-nidle  0) {
 /* Pop off the first resource */
 res = pop_resource(reslist);
 +if (reslist-ttl  (now - res-freed = reslist-ttl)) {
 +/* this res is expired - kill it */
 +rv = destroy_resource(reslist, res);
 +if (rv != APR_SUCCESS) {
 +apr_thread_mutex_unlock(reslist-listlock);
 +return rv;  /* FIXME: this might cause unnecessary
 fails */
 +}
 +continue;
 +}
 *resource = res-opaque;
 free_container(reslist, res);
 apr_thread_mutex_unlock(reslist-listlock);
 @@ -306,8 +315,7 @@
 }
 /* If we've hit our max, block until we're allowed to create
  * a new one, or something becomes free. */
 -else while (reslist-ntotal = reslist-hmax
 - reslist-nidle = 0) {
 +while (reslist-ntotal = reslist-hmax  reslist-nidle = 0) {
 if (reslist-timeout) {
 if ((rv = apr_thread_cond_timedwait(reslist-avail,
 reslist-listlock, reslist-timeout)) != APR_SUCCESS) {

 It's not perfect, but it has the advantage of neither disturbing
 the API/ABI, nor breaking promises to existing apps.


Looks good.

Cheers,
Henry

Re: apr_threadpool bogosity?

[Henry Jen]

Bill,

Sorry for late reply, I have been overwhelmed by JavaOne preparation.
I had reviewed the patch and gave my +1, just that I don't have comitter
access and won't be able to help. There is the other Solaris poll bug fix I
tried to get it in but no one seems to care even though Paul had say it
should be committed.

I am getting frustrated with APR, to be honest.

Cheers,
Henry


On Fri, May 2, 2008 at 11:36 AM, William A. Rowe, Jr. [EMAIL PROTECTED]
wrote:

 William A. Rowe, Jr. wrote:

  https://issues.apache.org/bugzilla/show_bug.cgi?id=42889
 
  which must be committed?
 
  https://issues.apache.org/bugzilla/attachment.cgi?id=20540
  https://issues.apache.org/bugzilla/attachment.cgi?id=20542
 
  I'm a little disturbed that the 1.3.0 discussion has been going on for
  about 2.5 weeks, and I just now noticed this flaw.  Once 1.3 is gone
  we can't *fix* the api, and can't expand it until 1.4
 

 More on the topic;

 https://issues.apache.org/bugzilla/show_bug.cgi?id=43876

 containing threadpool API patches here;

 https://issues.apache.org/bugzilla/attachment.cgi?id=21378

 without any closing feedback to Joe Mudd, submitter.

 The httpd project has a goal to tag on the 7th.  Work backwards, that's
 a tag here no later than the 4th.  I would hate to pull apr_threadpool,
 but if it can't be addressed today or tomorrow and some final decisions
 made on it's initial rollout, I'm tempted that we simply push it to 1.4.0,
 which would be a shame.

 Bill

Re: apr_threadpool bogosity?

[Henry Jen]

On Sat, May 3, 2008 at 1:57 AM, Henry Jen [EMAIL PROTECTED] wrote:

 Bill,

 Sorry for late reply, I have been overwhelmed by JavaOne preparation.
 I had reviewed the patch and gave my +1, just that I don't have comitter
 access and won't be able to help. There is the other Solaris poll bug fix I
 tried to get it in but no one seems to care even though Paul had say it
 should be committed.


One suggested me to point out the Solaris poll patch and Paul's comment, and
hopefully one dear comitter would take action.

The patch is attached to the issue:
https://issues.apache.org/bugzilla/show_bug.cgi?id=43000

Patch is here:
https://issues.apache.org/bugzilla/attachment.cgi?id=21136

Mails archive related:
http://mail-archives.apache.org/mod_mbox/apr-dev/200804.mbox/[EMAIL PROTECTED]
http://mail-archives.apache.org/mod_mbox/apr-dev/200712.mbox/[EMAIL PROTECTED]

Cheers,
Henry



 I am getting frustrated with APR, to be honest.

 Cheers,
 Henry



 On Fri, May 2, 2008 at 11:36 AM, William A. Rowe, Jr. [EMAIL PROTECTED]
 wrote:

  William A. Rowe, Jr. wrote:
 
   https://issues.apache.org/bugzilla/show_bug.cgi?id=42889
  
   which must be committed?
  
   https://issues.apache.org/bugzilla/attachment.cgi?id=20540
   https://issues.apache.org/bugzilla/attachment.cgi?id=20542
  
   I'm a little disturbed that the 1.3.0 discussion has been going on for
   about 2.5 weeks, and I just now noticed this flaw.  Once 1.3 is gone
   we can't *fix* the api, and can't expand it until 1.4
  
 
  More on the topic;
 
  https://issues.apache.org/bugzilla/show_bug.cgi?id=43876
 
  containing threadpool API patches here;
 
  https://issues.apache.org/bugzilla/attachment.cgi?id=21378
 
  without any closing feedback to Joe Mudd, submitter.
 
  The httpd project has a goal to tag on the 7th.  Work backwards, that's
  a tag here no later than the 4th.  I would hate to pull apr_threadpool,
  but if it can't be addressed today or tomorrow and some final decisions
  made on it's initial rollout, I'm tempted that we simply push it to
  1.4.0,
  which would be a shame.
 
  Bill
 

Committer love needed

[Henry Jen]

As the new branches got create, I am wondering if anyone can have a look at
this patch.

Also there is a patch for thread pool at bugzilla[1] I think is worth to
commit.

[1] https://issues.apache.org/bugzilla/show_bug.cgi?id=43876

Cheers,
Henry

-- Forwarded message --
From: Henry Jen [EMAIL PROTECTED]
Date: Mon, Dec 3, 2007 at 11:22 PM
Subject: [PATCH] Solaris port fix (bug 43000)
To: APR Developer List dev@apr.apache.org


Hi,

It's holiday season and things may be moving slower than it usually
is. The patch have been available for a while, and it fixes the port
code for Solaris which I really like to get it in.

It could be somewhat controversial for the atomic code used to defer
the port_associate in apr_pollset_add in order to pass the testpoll
code, which is the needed fix if we consider the semantic of
apr_pollset_poll should be giving accumulated events; Otherwise, we
should revise the test/testpoll.c.

With that said, I break the patch into two, the first port_1203-1.diff
fix bugs of:

1. Return APR_SUCCESS if the poll removed from pollset is in the
add_ring, which means the poll had been added but is current
disassociated because an event.
2. In apr_pollcb_poll, call apr_pollcb_add to ensure port_associate.

And the port_1203-2.diff is the fix to pass testpoll given current
situation, which is less a concern as I consider in most apps,
apr_pollset_poll is called in a loop.

Any comments are welcomed.

Cheers,
Henry
Index: poll/unix/port.c
===
--- poll/unix/port.c	(revision 600793)
+++ poll/unix/port.c	(working copy)
@@ -192,6 +192,7 @@
 pfd_elem_t *ep;
 apr_status_t rv = APR_SUCCESS;
 int res;
+int err;
 
 pollset_lock_rings();
 
@@ -205,6 +206,7 @@
 res = port_dissociate(pollset-port_fd, PORT_SOURCE_FD, fd);
 
 if (res  0) {
+err = errno;
 rv = APR_NOTFOUND;
 }
 
@@ -233,6 +235,9 @@
 APR_RING_REMOVE(ep, link);
 APR_RING_INSERT_TAIL((pollset-dead_ring),
  ep, pfd_elem_t, link);
+if (ENOENT == err) {
+rv = APR_SUCCESS;
+}
 break;
 }
 }
@@ -464,6 +469,7 @@
 if (rv) {
 return rv;
 }
+rv = apr_pollcb_add(pollcb, pollfd);
 }
 }
 
--- poll/unix/port_1.c	Mon Dec  3 21:37:14 2007
+++ poll/unix/port.c	Mon Dec  3 21:37:41 2007
@@ -15,6 +15,7 @@
  */
 
 #include apr_arch_poll_private.h
+#include apr_atomic.h
 
 #ifdef POLLSET_USES_PORT
 
@@ -80,6 +81,8 @@
 /* A ring of pollfd_t where rings that have been _remove'd but
might still be inside a _poll */
 APR_RING_HEAD(pfd_dead_ring_t, pfd_elem_t) dead_ring;
+/* number of threads in poll */
+volatile apr_uint32_t waiting;
 };
 
 static apr_status_t backend_cleanup(void *p_)
@@ -110,6 +113,7 @@
 return APR_ENOTIMPL;
 }
 #endif
+(*pollset)-waiting = 0;
 (*pollset)-nelts = 0;
 (*pollset)-nalloc = size;
 (*pollset)-flags = flags;
@@ -168,16 +172,22 @@
 fd = descriptor-desc.f-filedes;
 }
 
-res = port_associate(pollset-port_fd, PORT_SOURCE_FD, fd, 
- get_event(descriptor-reqevents), (void *)elem);
+if (apr_atomic_read32(pollset-waiting)) {
+res = port_associate(pollset-port_fd, PORT_SOURCE_FD, fd, 
+ get_event(descriptor-reqevents), (void *)elem);
 
-if (res  0) {
-rv = APR_ENOMEM;
-APR_RING_INSERT_TAIL((pollset-free_ring), elem, pfd_elem_t, link);
-}
+if (res  0) {
+rv = APR_ENOMEM;
+APR_RING_INSERT_TAIL((pollset-free_ring), elem, pfd_elem_t, link);
+}
+else {
+pollset-nelts++;
+APR_RING_INSERT_TAIL((pollset-query_ring), elem, pfd_elem_t, link);
+}
+} 
 else {
 pollset-nelts++;
-APR_RING_INSERT_TAIL((pollset-query_ring), elem, pfd_elem_t, link);
+APR_RING_INSERT_TAIL((pollset-add_ring), elem, pfd_elem_t, link);
 }
 
 pollset_unlock_rings();
@@ -273,6 +283,8 @@
 
 pollset_lock_rings();
 
+apr_atomic_inc32(pollset-waiting);
+
 while (!APR_RING_EMPTY((pollset-add_ring), pfd_elem_t, link)) {
 ep = APR_RING_FIRST((pollset-add_ring));
 APR_RING_REMOVE(ep, link);
@@ -296,6 +308,9 @@
 ret = port_getn(pollset-port_fd, pollset-port_set, pollset-nalloc,
 nget, tvptr);
 
+/* decrease the waiting ASAP to reduce the window for calling 
+   port_associate within apr_pollset_add() */
+apr_atomic_dec32(pollset-waiting);
 (*num) = nget;
 
 if (ret == -1) {

Re: apr_pollset_poll return value (APR_EINTR) on solaris

[Henry Jen]

For port.c support on Solaris, there is also this outstanding issue:

http://issues.apache.org/bugzilla/show_bug.cgi?id=43000

The patch had been reviewed and waiting for someone to commit.

Cheers,
Henry


On Feb 12, 2008 1:26 PM, Basant Kukreja [EMAIL PROTECTED] wrote:
 After incorporating the sugestion by Lucian, the following patch works for me.

 -
 --- orghttpd-2.2.6/srclib/apr/poll/unix/port.c  Fri Apr 13 13:54:13 2007
 +++ httpd-2.2.6/srclib/apr/poll/unix/port.c Tue Feb 12 13:12:38 2008
 @@ -295,12 +295,7 @@

  if (ret == -1) {
  (*num) = 0;
 -if (errno == ETIME || errno == EINTR) {
 -rv = APR_TIMEUP;
 -}
 -else {
 -rv = APR_EGENERAL;
 -}
 +rv = apr_get_netos_error();
  }
  else if (nget == 0) {
  rv = APR_TIMEUP;
 -

 Regards,
 Basant.



 On Tue, Feb 12, 2008 at 04:10:32PM -0500, Jim Jagielski wrote:
 
  On Feb 12, 2008, at 3:18 PM, Lucian Adrian Grijincu wrote:
 
  This is the code in question.
 
 if (ret == -1) {
 (*num) = 0;
 if (errno == ETIME || errno == EINTR) {
 rv = APR_TIMEUP;
 }
 else {
 rv = APR_EGENERAL;
 }
 }
 
  I don't really like the APR_EGENERAL in the else either.
  Shouldn't this be something like:
  if (ret == -1)
  {
(*num) = 0;
rv = apr_get_netos_error();
  }
 
  and let apr_get_netos_error handle OS to APR errors consistent with
  other architectures (select.c, poll.c)
 
 
  +1
 
  Basant, can you confirm this works as expected?
 

Re: Hard to build httpd APR on OS X Leopard

[Henry Jen]

On Jan 11, 2008 12:20 PM, Tim Bray [EMAIL PROTECTED] wrote:

 But... I couldn't get httpd to build with my new APR living in /usr/
 local/apr.  I think this is because httpd's configure writes build/
 config_vars.mk including this:

 LDFLAGS = -L/usr/lib -L/usr/local/apr/lib

 So, it's irrelevant how you go about building APR, you're gonna get
 the one that Leopard ships in /usr/lib (and hey, it's nice that
 Leopard ships with APR).

 I tried a few ./configure options but couldn't find one to force it to
 change the -L, so I eventually went and edited build/config_vars.mk,
 blecch.

 But then httpd wouldn't build, on the final -o httpd step I got

 Undefined symbols:
_apr_socket_sendfile, referenced from:
_ap_hack_apr_socket_sendfile in libmain.a(exports.o)
_sendfile_it_all in libmain.a(core_filters.o)
 ld: symbol(s) not found

 Hokay... I suspect it's most likely that's I'm just ignorant of
 something obvious that Everybody Knows.  If so, what is it?


My guess is that you are not using the matching header files, perhaps
you only fixed the -L option, but not the -I option for the header
file.

HTH,
Henry

 It does seem like it shouldn't be this hard.  -Tim

[PATCH] Solaris port fix (bug 43000)

[Henry Jen]

Hi,

It's holiday season and things may be moving slower than it usually
is. The patch have been available for a while, and it fixes the port
code for Solaris which I really like to get it in.

It could be somewhat controversial for the atomic code used to defer
the port_associate in apr_pollset_add in order to pass the testpoll
code, which is the needed fix if we consider the semantic of
apr_pollset_poll should be giving accumulated events; Otherwise, we
should revise the test/testpoll.c.

With that said, I break the patch into two, the first port_1203-1.diff
fix bugs of:

1. Return APR_SUCCESS if the poll removed from pollset is in the
add_ring, which means the poll had been added but is current
disassociated because an event.
2. In apr_pollcb_poll, call apr_pollcb_add to ensure port_associate.

And the port_1203-2.diff is the fix to pass testpoll given current
situation, which is less a concern as I consider in most apps,
apr_pollset_poll is called in a loop.

Any comments are welcomed.

Cheers,
Henry
Index: poll/unix/port.c
===
--- poll/unix/port.c	(revision 600793)
+++ poll/unix/port.c	(working copy)
@@ -192,6 +192,7 @@
 pfd_elem_t *ep;
 apr_status_t rv = APR_SUCCESS;
 int res;
+int err;
 
 pollset_lock_rings();
 
@@ -205,6 +206,7 @@
 res = port_dissociate(pollset-port_fd, PORT_SOURCE_FD, fd);
 
 if (res  0) {
+err = errno;
 rv = APR_NOTFOUND;
 }
 
@@ -233,6 +235,9 @@
 APR_RING_REMOVE(ep, link);
 APR_RING_INSERT_TAIL((pollset-dead_ring),
  ep, pfd_elem_t, link);
+if (ENOENT == err) {
+rv = APR_SUCCESS;
+}
 break;
 }
 }
@@ -464,6 +469,7 @@
 if (rv) {
 return rv;
 }
+rv = apr_pollcb_add(pollcb, pollfd);
 }
 }
 
--- poll/unix/port_1.c	Mon Dec  3 21:37:14 2007
+++ poll/unix/port.c	Mon Dec  3 21:37:41 2007
@@ -15,6 +15,7 @@
  */
 
 #include apr_arch_poll_private.h
+#include apr_atomic.h
 
 #ifdef POLLSET_USES_PORT
 
@@ -80,6 +81,8 @@
 /* A ring of pollfd_t where rings that have been _remove'd but
might still be inside a _poll */
 APR_RING_HEAD(pfd_dead_ring_t, pfd_elem_t) dead_ring;
+/* number of threads in poll */
+volatile apr_uint32_t waiting;
 };
 
 static apr_status_t backend_cleanup(void *p_)
@@ -110,6 +113,7 @@
 return APR_ENOTIMPL;
 }
 #endif
+(*pollset)-waiting = 0;
 (*pollset)-nelts = 0;
 (*pollset)-nalloc = size;
 (*pollset)-flags = flags;
@@ -168,16 +172,22 @@
 fd = descriptor-desc.f-filedes;
 }
 
-res = port_associate(pollset-port_fd, PORT_SOURCE_FD, fd, 
- get_event(descriptor-reqevents), (void *)elem);
+if (apr_atomic_read32(pollset-waiting)) {
+res = port_associate(pollset-port_fd, PORT_SOURCE_FD, fd, 
+ get_event(descriptor-reqevents), (void *)elem);
 
-if (res  0) {
-rv = APR_ENOMEM;
-APR_RING_INSERT_TAIL((pollset-free_ring), elem, pfd_elem_t, link);
-}
+if (res  0) {
+rv = APR_ENOMEM;
+APR_RING_INSERT_TAIL((pollset-free_ring), elem, pfd_elem_t, link);
+}
+else {
+pollset-nelts++;
+APR_RING_INSERT_TAIL((pollset-query_ring), elem, pfd_elem_t, link);
+}
+} 
 else {
 pollset-nelts++;
-APR_RING_INSERT_TAIL((pollset-query_ring), elem, pfd_elem_t, link);
+APR_RING_INSERT_TAIL((pollset-add_ring), elem, pfd_elem_t, link);
 }
 
 pollset_unlock_rings();
@@ -273,6 +283,8 @@
 
 pollset_lock_rings();
 
+apr_atomic_inc32(pollset-waiting);
+
 while (!APR_RING_EMPTY((pollset-add_ring), pfd_elem_t, link)) {
 ep = APR_RING_FIRST((pollset-add_ring));
 APR_RING_REMOVE(ep, link);
@@ -296,6 +308,9 @@
 ret = port_getn(pollset-port_fd, pollset-port_set, pollset-nalloc,
 nget, tvptr);
 
+/* decrease the waiting ASAP to reduce the window for calling 
+   port_associate within apr_pollset_add() */
+apr_atomic_dec32(pollset-waiting);
 (*num) = nget;
 
 if (ret == -1) {

Re: APR 2.0 proposals

[Henry Jen]

On Nov 29, 2007 12:48 PM, Bojan Smojver [EMAIL PROTECTED] wrote:
 On Fri, 2007-11-23 at 10:22 -0600, William A. Rowe, Jr. wrote:
  are we looking at a 1.3 iteration before we
  make such radical changes as 2.0?  If so, when?  I'd love to see 1.3
  come together over the next month or two for those who don't want to
  wait on/depend upon such a major release bump.

 I think that would be a good idea, given that 1.3 is compatible with
 1.2.


+1.

 In terms of when, do we know what in 1.3 is in not-to-be-released-yet
 status?


There are two issues I believed is ready to been committed, but would
need a committer to sponsor them.

http://issues.apache.org/bugzilla/show_bug.cgi?id=43000. I went
through this patch with Paul at ApacheCon.
http://issues.apache.org/bugzilla/show_bug.cgi?id=43876. Joe had this
patch submitted for a while, and we both think the patch is good to
go.

Cheers,
Henry

Re: Please read: Fix for APR bug

[Henry Jen]

On Nov 26, 2007 4:25 PM, Erik Lotspeich [EMAIL PROTECTED] wrote:
 Hi all:

 I've posted a few messages regarding this topic.  I believe that there
 must be someone interested in this topic who wrote the apr_pools.c
 originally or has worked with apr_pool_note_subprocess().  If not, then I
 suppose that would explain the lack of interest in this problem.


It can be hard sometimes to get attention you would like to have,
especially when others are not experiencing the problem immediately.
I read the thread, however I did not spend time in this area, so I
don't think I am qualified to reply.

 To summarize, I believe that there is a bug either in apr_pools.c (in the
 function free_proc_chain()) or in apr_proc_wait().  I posted a patch to
 this list with a fix that does work.  I didn't begin to dive into
 apr_proc_wait() (which may be the true source of the problem) until I got
 some feedback from the developers on this list.


After a little study, I guess the question is that when the return
value of apr_proc_wait is not APR_CHILD_NOTDONE, does it guarantee
there is a subprocess still running? My reading to man page of waitpid
on Solaris does not indicate that.

So, flip the condition may solve your problem but may cause other issues.

Anyhow, others may know better than I do.

Cheers,
Henry

 Any response would be greatly appreciated.

 Regards,

 Erik.

 On Sat, 24 Nov 2007, Erik Lotspeich wrote:

  Hi:
 
  I continued my investigation as to the reason why apr_pool_note_subprocess()
  is not working for me.  It seems that there is a bug in apr_pools.c in the
  function free_proc_chain().  Here's my patch:
 
  --- apr_pools.c 2007-11-24 23:06:12.0 -0800
  +++ apr_pools.c+2007-11-24 23:06:01.0 -0800
  @@ -2118,7 +2118,7 @@
  #ifndef NEED_WAITPID
  /* Pick up all defunct processes */
  for (pc = procs; pc; pc = pc-next) {
  -if (apr_proc_wait(pc-proc, NULL, NULL, APR_NOWAIT) !=
  APR_CHILD_NOTDONE)
  +if (apr_proc_wait(pc-proc, NULL, NULL, APR_NOWAIT) ==
  APR_CHILD_DONE)
  pc-kill_how = APR_KILL_NEVER;
  }
  #endif /* !defined(NEED_WAITPID) */
 
  It may be true that apr_proc_wait is the original source of the problem.  In
  theory, both the previous and patched versions of the code above are
  equivalent.  In reality, the doxygen documentation is incomplete.
  apr_proc_wait can return codes other than APR_CHILD_DONE and
  APR_CHILD_NOTDONE.  In my case, the process I to be checked was actually
  running, but an APR_CHILD_NOTDONE code wasn't being set!
 
  Any questions or comments would be appreciated.
 
  Regards,
 
  Erik.
 

Re: [Vote] apr-util-1.2.12 candidate in /dev/dist/

[Henry Jen]

Hi,

I tested with the tags/1.2.12 from svn, and got the following on
Solaris Express, with built-in iconv instead of apr-iconv.

testpass:  SUCCESS
testmd4 :  SUCCESS
testmd5 :  SUCCESS
testdbd :  SUCCESS
testdate:  SUCCESS
testxml :  SUCCESS
testxlate   :  FAILED 1 of 1
Line 63: expected 22, but saw 0
testrmm :  SUCCESS
testdbm :  SUCCESS
testqueue   :  SUCCESS
testreslist :  .SUCCESS
Failed TestsTotal   FailFailed %
===
testxlate   1  1100.00%

This is caused by the ISO-8859-1/ISO-8859-2 translate, which according
to man page is not supported.

Thoughts?

Cheers,
Henry

On Nov 21, 2007 12:32 PM, Ruediger Pluem [EMAIL PROTECTED] wrote:


 On 11/21/2007 01:45 AM, William A. Rowe, Jr. wrote:
  Please provide your input to release the tarball candidate
 
[+1] APR-util 1.2.12
 
  voting closes Friday afternoon/evening.  Windows .zip's with all those
  pesky .mak files on their way by morning.
 
  Bill
 
 

 Signatures: OK
 md5sums   : OK

 All tests past with the exception of the following error for sqlite2:

  prepared select 
 Prepare statement failed!
 (null)
 Error in prepared select: rc=70023

  prepared query 
 Prepare statement failed!
 (null)
 Error in prepared query: rc=70023

 But this is no regression.

 Test were performed on

 Solaris 8
 Solaris 9
 Solaris 10
 Red Hat AS 4 64 Bit
 OpenSuSE 10.1 64 Bit
 OpenSuSE 10.2 32 Bit

 So +1 from me on apr-util 1.2.12

 Regards

 Rüdiger

[PATCH] URI handling more compliant to RFC3986 [was Re: ApacheCon US]

[Henry Jen]

On Oct 11, 2007 9:43 PM, Henry Jen [EMAIL PROTECTED] wrote:

 On 10/5/07, Paul Querna [EMAIL PROTECTED] wrote:
  William A. Rowe, Jr. wrote:
   Henry Jen wrote:
   Hi,
  
   ApacheCon US is a month away, and this year I get to attend the event, 
   yeah!
  
   Wondering if there is going to be a hackathon or some sort of activity
   for APR/APR-util developers to get together?
  
   I would sure like to see it!  Since Tuesday gets busy with several special
   events, who's up for trying to be there on Monday?
 
  I'll be around... I would like to hammer out the APR Events stuff that
  davi commited to a branch, and merge it into trunk:
  http://svn.apache.org/repos/asf/apr/apr/branches/evenset/
 
  Anything else people would like to see?
 

 I will be around as well.
 What I would like to discuss is a few changes to apr_uri to be more
 accurate to RFC 3986 compliant, particular for the path support.

 Per RFC 3986,

 URI   = scheme : hier-part [ ? query ] [ # fragment ]

 hier-part = // authority path-abempty
  / path-absolute
  / path-rootless
  / path-empty

 I am expecting file:./tmp/tnt.pdf to get a scheme file and path
 ./tmp/tnt.pdf, but I got:

 [EMAIL PROTECTED]:~/prj/test/c$ ./apr_uri_test file:./tmp/tnt.pdf
 Parsing file:./tmp/tnt.pdf 
   scheme:   n/a
   hostinfo: n/a
   user: n/a
   password: n/a
   hostname: n/a
   port_str: n/a
   path: file:./tmp/tnt.pdf
   query:n/a
   fragment: n/a
   hostent:  0
   port: 0
   is_initialized:   1
   dns_looked_up:0
   dns_resolved: 0

Index: uri/apr_uri.c
===
--- uri/apr_uri.c	(revision 582411)
+++ uri/apr_uri.c	(working copy)
@@ -92,7 +92,13 @@
 unsigned flags)
 {
 char *ret = ;
+char *scheme = NULL;
 
+if (uptr-scheme) {
+scheme = apr_pstrcat(p, uptr-scheme, :, NULL);
+}
+
+
 /* If suppressing the site part, omit both user name  scheme://hostname */
 if (!(flags  APR_URI_UNP_OMITSITEPART)) {
 
@@ -129,29 +135,15 @@
  uptr-port == 0 ||
  uptr-port == apr_uri_port_of_scheme(uptr-scheme));
 
-if (uptr-scheme) {
-ret = apr_pstrcat(p,
-  uptr-scheme, ://, ret,
-  lbrk, uptr-hostname, rbrk,
-  is_default_port ?  : :,
-  is_default_port ?  : uptr-port_str,
-  NULL);
-}
-else {
-/* A violation of RFC2396, but it is clear from section 3.2
- * that the : belongs above to the scheme, while // belongs
- * to the authority, so include the authority prefix while
- * omitting the scheme: that the user neglected to pass us.
- */
-ret = apr_pstrcat(p,
-  //, ret, lbrk, uptr-hostname, rbrk,
-  is_default_port ?  : :,
-  is_default_port ?  : uptr-port_str,
-  NULL);
-}
+ret = apr_pstrcat(p, //, ret, lbrk, uptr-hostname, rbrk,
+is_default_port ?  : :,
+is_default_port ?  : uptr-port_str,
+NULL);
 }
 }
 
+ret = apr_pstrcat(p, scheme ? scheme : , ret, NULL);
+
 /* Should we suppress all path info? */
 if (!(flags  APR_URI_UNP_OMITPATHINFO)) {
 /* Append path, query and fragment strings: */
@@ -324,12 +316,17 @@
 while ((uri_delims[*(unsigned char *)s]  NOTEND_SCHEME) == 0) {
 ++s;
 }
-/* scheme must be non-empty and followed by :// */
-if (s == uri || s[0] != ':' || s[1] != '/' || s[2] != '/') {
+/* scheme must be non-empty and followed by : */
+if (s == uri || s[0] != ':') {
 goto deal_with_path;/* backwards predicted taken! */
 }
 
 uptr-scheme = apr_pstrmemdup(p, uri, s - uri);
+if (s[1] != '/' || s[2] != '/') {
+uri = s + 1;
+goto deal_with_path;
+}
+
 s += 3;
 
 deal_with_authority:
Index: test/testuri.c
===
--- test/testuri.c	(revision 582411)
+++ test/testuri.c	(working copy)
@@ -95,6 +95,30 @@
 //www.apache.org/,
 0, NULL, www.apache.org, NULL, NULL, www.apache.org, NULL, /, NULL, NULL, 0
 },
+{
+file:image.jpg,
+0, file, NULL, NULL, NULL, NULL, NULL, image.jpg, NULL, NULL, 0
+},
+{
+file:/image.jpg,
+0, file, NULL, NULL, NULL, NULL, NULL, /image.jpg, NULL, NULL, 0
+},
+{
+file:///image.jpg,
+0, file, , NULL, NULL, , NULL, /image.jpg, NULL, NULL, 0

Re: [PATCH] UDP Bucket Support

[Henry Jen]

On Nov 12, 2007 6:41 PM, Davi Arnaut [EMAIL PROTECTED] wrote:
 Paul Querna wrote:
  Adds: apr_bucket_socket_get_peer() and has the bucket read call recvfrom().
 
  Based on an earlier patch from Issac Goldstand.
 

 [..]

  -rv = apr_socket_recv(p, buf, len);
  +apr_socket_type_get(p, sd_type);
  +if (sd_type == SOCK_STREAM) {
  +rv = apr_socket_recv(p, buf, len);
  +} else {
  +/* Is socket connected? */
  +if (apr_socket_addr_get(baton-peer, APR_REMOTE, p) != 
  APR_SUCCESS) {
  +rv = apr_socket_recv(p, buf, len);
  +} else {
  +if (!baton-peer) {
  +baton-peer = apr_bucket_alloc(sizeof(*baton-peer), 
  a-list);
  +}
  +/* Caller is responsible for detecting peer on his own if 
  needed */
  +rv = apr_socket_recvfrom(baton-peer, p, 0, buf, len);
  +}
  +}

 Why not cache the the peer and avoid a call to apr_socket_addr_get on
 every read?


UDP traffic can come from different peers.
I wonder if we can simply call apr_socket_recvfrom for all non-stream protocols?

Cheers,
Henry

[PATCH] apr_poll[set|cb]_remove should returns APR_SUCCESS instead of APR_NOTFOUND with testpoll

[Henry Jen]

Hi,

Attached is a fix for the poll implementation on Solaris using port. The 
issue is that when calling port_dissociate with a file descriptor has 
not been associated, port_dissociate will return -1 and set errno to ENOENT.


The apr_pollcb_poll should also re-associate the fd with port.

Per the man page for port_dissociate:

The port_dissociate() function will fail if:

 EACCESThe process is not the owner of the association.


 ENOENTThe specified object is not  associated  with  the
   port.


Cheers,
Henry

Index: poll/unix/port.c
===
--- poll/unix/port.c	(revision 591399)
+++ poll/unix/port.c	(working copy)
@@ -192,6 +192,7 @@
 pfd_elem_t *ep;
 apr_status_t rv = APR_SUCCESS;
 int res;
+int err;
 
 pollset_lock_rings();
 
@@ -205,6 +206,7 @@
 res = port_dissociate(pollset-port_fd, PORT_SOURCE_FD, fd);
 
 if (res  0) {
+err = errno;
 rv = APR_NOTFOUND;
 }
 
@@ -233,6 +235,9 @@
 APR_RING_REMOVE(ep, link);
 APR_RING_INSERT_TAIL((pollset-dead_ring),
  ep, pfd_elem_t, link);
+if (ENOENT == err) {
+rv = APR_SUCCESS;
+}
 break;
 }
 }
@@ -464,6 +469,7 @@
 if (rv) {
 return rv;
 }
+rv = apr_pollcb_add(pollcb, pollfd);
 }
 }
 

[PATCH] testpoll fix for Solaris port backend [was: Re: Showstoppers to apr release(?)]

[Henry Jen]

Hi,

Attached is a patch fix the testpoll issues found on Solaris. It
combines the previous fix for removing a poll.

This patch basically minimize the timing window to make a
port_associate before someone is trying to retrieve events from the
port.

Cheers,
Henry


On 10/30/07, Henry Jen [EMAIL PROTECTED] wrote:
 On 10/30/07, Henry Jen [EMAIL PROTECTED] wrote:
  On 10/29/07, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
   William A. Rowe, Jr. wrote:
  
* Solaris 10, testpoll.c:314 we are failing with only APR_POLLOUT when 
we
  had expected both APR_POLLIN and APR_POLLOUT to be signaled.  Ideas?
  
 
  Seems like an issue with event port library, to associate with both
  POLLIN and POLLOUT event does not work on Solaris. I had created a
  simple test program as attached, and I noted that if we change the
  associate to POLLIN only, we will get the POLLIN event.
 
  Disable detection of port_create from configure.in to force using poll
  on Solaris pass the test.
 
  I am forwarding this to OpenSolaris developers and hopefully will get
  some explanation.
 
 

 After some more investigation and read into the man page more carefully,
 I found the following explains the issue. The following discussion is
 based on the test code attached with previous email.

 Quoted from the man page of port_associate:

   Objects of type PORT_SOURCE_FD  are  file  descriptors.  The
   event  types  for  PORT_SOURCE_FD  objects  are described in
   poll(2).  At most one event notification will  be  generated
   per  associated  file  descriptor.   For  example, if a file
   descriptor is associated with  a  port  for  the  POLLRDNORM
   event  and  data  is available on the file descriptor at the
   time the port_associate() function is called,  an  event  is
   immediately  sent to the port.

 This explains the first event only has POLLOUT flagged, as sendto is
 called after port_associate. By switching the order, we get an event
 with POLLIN and POLLOUT both flagged.

   If data is not yet available,
   one event is sent to the port when data first becomes avail-
   able.
  
   When an event for a PORT_SOURCE_FD object is retrieved,  the
   object  no  longer  has  an  association with the port.  The
   event can be processed without the possibility that  another
   thread  can retrieve a subsequent event for the same object.
   After processing of the file descriptor  is  completed,  the
   port_associate()  function  can be called to reassociate the
   object with the port.

 This indicates another call to port_associate is needed within the while
 loop to continue receive events interested. By adding port_associate
 into the while loop, the program will get the POLLIN event at the second
 attempt without changing the order of sendto call as mentioned above.

 So a hack to fix to testpoll.c is to call apr_pollset_add after send_msg.

 A better fix for Solaris, IMHO, is don't call port_associate in
 apr_pollset_add but leave it to apr_pollset_poll by putting the
 request into the add_ring. Attached patch fix get over this issue.
 However, testpoll still have other existing failures on my Solaris
 Developer Express system.

 testpoll: |Line 343: expected 0, but saw 70015
 |Line 523: expected 0, but saw 70015
 |Line 615: expected 0, but saw 70015
 FAILED 3 of 19
 Failed TestsTotal   FailFailed %
 ===
 testpoll   19  3 15.79%

 HTH,
 Henry


Index: poll/unix/port.c
===
--- poll/unix/port.c	(revision 591399)
+++ poll/unix/port.c	(working copy)
@@ -15,6 +15,7 @@
  */
 
 #include apr_arch_poll_private.h
+#include apr_atomic.h
 
 #ifdef POLLSET_USES_PORT
 
@@ -80,6 +81,8 @@
 /* A ring of pollfd_t where rings that have been _remove'd but
might still be inside a _poll */
 APR_RING_HEAD(pfd_dead_ring_t, pfd_elem_t) dead_ring;
+/* number of threads in poll */
+volatile apr_uint32_t waiting;
 };
 
 static apr_status_t backend_cleanup(void *p_)
@@ -110,6 +113,7 @@
 return APR_ENOTIMPL;
 }
 #endif
+(*pollset)-waiting = 0;
 (*pollset)-nelts = 0;
 (*pollset)-nalloc = size;
 (*pollset)-flags = flags;
@@ -168,16 +172,22 @@
 fd = descriptor-desc.f-filedes;
 }
 
-res = port_associate(pollset-port_fd, PORT_SOURCE_FD, fd, 
- get_event(descriptor-reqevents), (void *)elem);
+if (apr_atomic_read32(pollset-waiting)) {
+res = port_associate(pollset-port_fd, PORT_SOURCE_FD, fd, 
+ get_event(descriptor-reqevents), (void *)elem);
 
-if (res  0) {
-rv = APR_ENOMEM;
-APR_RING_INSERT_TAIL((pollset-free_ring), elem, pfd_elem_t, link);
-}
+if (res  0) {
+rv = APR_ENOMEM;
+APR_RING_INSERT_TAIL((pollset-free_ring), elem, pfd_elem_t, link);
+}
+else {
+pollset-nelts

Re: Any libtool/autoconf guru have 20 minutes to hack?

[Henry Jen]

Attached is a patch giving the -version-info, works for me.

However, I am not sure if this is enough, as in ccs/ces, the libtool
is called with -avoid-version, which makes me suspicious the version
is left out on purpose.

HTH,
Henry

On 10/29/07, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
 William A. Rowe, Jr. wrote:
  libapriconv trunk would appreciate a small tweak here;
 
  apr.exp  libapr-1.so libapriconv-1.solibaprutil-1.so
  aprutil.exp  libapr-1.so.0   libapriconv-1.so.0  libaprutil-1.so.0
  iconvlibapr-1.so.0.2.12  libapriconv-1.so.0.0.0
  libaprutil-1.so.0.2.12
  libapr-1.a   libapriconv-1.a libaprutil-1.a  pkgconfig
  libapr-1.la  libapriconv-1.lalibaprutil-1.la
 
  Note it's not identified as libapriconv-1.so.0.2.1 (trunk rev from
  api_version.h).
 
  I don't consider it a showstopper, but it would be nicer if we fixed before 
  rolling
  version 1.2.2.  The right hacker could do this in 1/10th the time it would 
  take me
  to figure this out :)

 ping?  any helper good at version decorations?

Index: configure.in
===
--- configure.in	(revision 589963)
+++ configure.in	(working copy)
@@ -90,6 +90,8 @@
 AC_SUBST(API_LIBNAME)
 API_INCPATH=apr-${API_MAJOR_VERSION}
 AC_SUBST(API_INCPATH)
+LT_VERSION=-version-info `$get_version libtool $version_hdr API`
+AC_SUBST(LT_VERSION)
 
 dnl
 dnl everthing is done. 
Index: lib/Makefile.in
===
--- lib/Makefile.in	(revision 589963)
+++ lib/Makefile.in	(working copy)
@@ -24,6 +24,7 @@
 iconv_ces.lo iconv_ces_euc.lo iconv_ces_iso2022.lo api_version.lo \
 iconv_ccs.lo exports.lo
 
+LT_VERSION = @LT_VERSION@
 # bring in rules.mk for standard functionality
 @INCLUDE_RULES@
 # replace build-outputs.mk

Fwd: Showstoppers to apr release(?)

[Henry Jen]

Forward to the list.

-- Forwarded message --
From: Henry Jen [EMAIL PROTECTED]
Date: Oct 30, 2007 7:59 PM
Subject: Re: Showstoppers to apr release(?)
To: William A. Rowe, Jr. [EMAIL PROTECTED]



 A better fix for Solaris, IMHO, is don't call port_associate in
 apr_pollset_add but leave it to apr_pollset_poll by putting the
 request into the add_ring. Attached patch fix get over this issue.

A second thought found this may not be a good idea. Given a thread is
blocking on apr_pollset_poll, then another thread doing a
apr_pollset_add, I suppose when the desired event for the new socket
arrived should unblock the waiting thread.

Is that understanding correct? If so, the proposed patch will fail
that scenario.

Cheers,
Henry

Re: ApacheCon US

[Henry Jen]

On 10/5/07, Paul Querna [EMAIL PROTECTED] wrote:
 William A. Rowe, Jr. wrote:
  Henry Jen wrote:
  Hi,
 
  ApacheCon US is a month away, and this year I get to attend the event, 
  yeah!
 
  Wondering if there is going to be a hackathon or some sort of activity
  for APR/APR-util developers to get together?
 
  I would sure like to see it!  Since Tuesday gets busy with several special
  events, who's up for trying to be there on Monday?

 I'll be around... I would like to hammer out the APR Events stuff that
 davi commited to a branch, and merge it into trunk:
 http://svn.apache.org/repos/asf/apr/apr/branches/evenset/

 Anything else people would like to see?


I will be around as well.
What I would like to discuss is a few changes to apr_uri to be more
accurate to RFC 3986 compliant, particular for the path support.

Per RFC 3986,

URI   = scheme : hier-part [ ? query ] [ # fragment ]

hier-part = // authority path-abempty
 / path-absolute
 / path-rootless
 / path-empty

I am expecting file:./tmp/tnt.pdf to get a scheme file and path
./tmp/tnt.pdf, but I got:

[EMAIL PROTECTED]:~/prj/test/c$ ./apr_uri_test file:./tmp/tnt.pdf
Parsing file:./tmp/tnt.pdf 
  scheme:   n/a
  hostinfo: n/a
  user: n/a
  password: n/a
  hostname: n/a
  port_str: n/a
  path: file:./tmp/tnt.pdf
  query:n/a
  fragment: n/a
  hostent:  0
  port: 0
  is_initialized:   1
  dns_looked_up:0
  dns_resolved: 0

ApacheCon US

[Henry Jen]

Hi,

ApacheCon US is a month away, and this year I get to attend the event, yeah!

Wondering if there is going to be a hackathon or some sort of activity
for APR/APR-util developers to get together?

Cheers,
Henry

Re: [PATCH] apr_threadpool

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry,

thank you for your submission.

http://svn.apache.org/viewvc?view=revrevision=492362

Please remember, the API is flexible until 1.3.0 (or 2.0.0) is tagged and
released, at which point the new files become subject to the (rather strict)
versioning rules.



Hi Bill,

Happy 2007! This is great, thank you for all the help to get this committed.

Attached is a small patch, it includes two modifications:

1. Fix the assert, which should only for preventing task from owner 
itself calling cancel.


2. Add a apr_thread_pool_task_owner_get API to retrieve the owner of the 
task. This is convenient and can, in many cases, eliminate the need to 
create a structure to use as parameters.


Cheers,
Henry

Index: include/apr_thread_pool.h
===
--- include/apr_thread_pool.h	(revision 492376)
+++ include/apr_thread_pool.h	(working copy)
@@ -226,6 +226,15 @@
  */
 APR_DECLARE(apr_size_t) apr_thread_pool_threshold_get(apr_thread_pool_t * me);
 
+/**
+ * Get owner of the task currently been executed by the thread. 
+ * @param thd The thread is executing a task 
+ * @param owner Pointer to receive owner of the task.
+ * @return APR_SUCCESS if the owner is retrieved successfully
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_task_owner_get(apr_thread_t * thd,
+ void **owner);
+
 #ifdef __cplusplus
 #if 0
 {
Index: misc/apr_thread_pool.c
===
--- misc/apr_thread_pool.c	(revision 492376)
+++ misc/apr_thread_pool.c	(working copy)
@@ -233,6 +233,7 @@
 while (NULL != task  !me-terminated) {
 elt-current_owner = task-owner;
 apr_thread_mutex_unlock(me-lock);
+apr_thread_data_set(task, apr_thread_pool_task, NULL, t);
 task-func(t, task-param);
 apr_thread_mutex_lock(me-lock);
 APR_RING_INSERT_TAIL(me-recycled_tasks, task,
@@ -598,6 +599,10 @@
 apr_thread_mutex_lock(me-lock);
 elt = APR_RING_FIRST(me-busy_thds);
 while (elt != APR_RING_SENTINEL(me-busy_thds, apr_thread_list_elt, link)) {
+if (elt-current_owner != owner) {
+elt = APR_RING_NEXT(elt, link);
+continue;
+}
 #ifndef NDEBUG
 /* make sure the thread is not the one calling tasks_cancel */
 apr_os_thread_t *os_thread;
@@ -609,10 +614,6 @@
 assert(!apr_os_thread_equal(apr_os_thread_current(), *os_thread));
 #endif
 #endif
-if (elt-current_owner != owner) {
-elt = APR_RING_NEXT(elt, link);
-continue;
-}
 while (elt-current_owner == owner) {
 apr_thread_mutex_unlock(me-lock);
 apr_sleep(200 * 1000);
@@ -812,6 +813,26 @@
 return ov;
 }
 
+APR_DECLARE(apr_status_t) apr_thread_pool_task_owner_get(apr_thread_t * thd,
+ void **owner)
+{
+apr_status_t rv;
+apr_thread_pool_task_t * task;
+
+rv = apr_thread_data_get((void**) task, apr_thread_pool_task, thd);
+if (rv != APR_SUCCESS) {
+return rv;
+}
+
+if (!task) {
+*owner = NULL;
+return APR_BADARG;
+}
+
+*owner = task-owner;
+return APR_SUCCESS;
+}
+
 #endif /* APR_HAS_THREADS */
 
 /* vim: set ts=4 sw=4 et cin tw=80: */

Re: [PATCH] apr_threadpool

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:
 
+/**
+ * Get owner of the task currently been executed by the thread. 
+ * @param thd The thread is executing a task 
+ * @param owner Pointer to receive owner of the task.

+ * @return APR_SUCCESS if the owner is retrieved successfully
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_task_owner_get(apr_thread_t * thd,
+ void **owner);


the void* containing what(???)  Can we define an owner a bit more deliberately?



It is the pointer passed in when push/top/schedule a task, simply a 
opaque pointer to present the owner of the task.


Cheers,
Henry

Adding a cleanup fucntion for thread pool task

[Henry Jen]

Hi,

Happy holiday to all. :-)

With the thread pool support patch[1] sit in bugzilla, recently I found 
it needs to have a cleanup function for the task pushed/scheduled. Also, 
it would be useful for a worker thread being able to get the owner.


So I would like to propose a change and an addition,

Addition:

/**
 * Get owner of the task currently been executed by the thread.
 * @param thd The thread is executing a task
 * @param owner Pointer to receive owner of the task.
 * @return APR_SUCCESS if the owner is retrieved successfully
 */
APR_DECLARE(apr_status_t) apr_thread_pool_task_owner_get(apr_thread_t * 
thd, void **owner);


Change:

APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t * me,
   apr_thread_start_t func,
   void *param,
   apr_byte_t priority,
   void *owner);

to either

APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t * me,
  apr_thread_start_t func,
  void *param,
  apr_byte_t priority,
  void *owner,
  apr_status_t (*cleanup) (void*));

or

APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t * me,
  apr_thread_start_t func,
  void *param,
  void *owner,
  ...);
Where ... would be optional parameters provided as

apr_byte_t priority, apr_status_t (*cleanup) (void*)


The later maybe a good for potential parameters, but I don't see the 
need for now.


Comments?

[1] http://issues.apache.org/bugzilla/show_bug.cgi?id=41061

Cheers,
Henry

Re: [PATCH] rewrite expat detection

[Henry Jen]

Dr. Peter Poeml wrote:

On Thu, Nov 16, 2006 at 03:12:28 +, Joe Orton wrote:
This is a rewrite of the expat detection in apr-util to fix lingering 
problems like PR 28205 caused by attempting to detect presence of 
libraries/headers using test -f:

[...]

- aiming for trunk not the 1.2.x branch!


Meanwhile, is there a point in the applying the attached patch, to help
the 1.2.8 branch work in one more case (missing .la file)?



I can see there are several patch floating around, bug 40892[1] was 
filed with this one[2] use AC_CHECK_LIB instead of shaky test on reading 
a file.


[1] http://issues.apache.org/bugzilla/show_bug.cgi?id=40892
[2] http://issues.apache.org/bugzilla/attachment.cgi?id=19086

Cheers,
Henry

Re: [PATCH] rewrite expat detection

[Henry Jen]

Joe Orton wrote:

On Fri, Nov 17, 2006 at 11:38:42AM -0800, Henry Jen wrote:

Joe Orton wrote:
This is a rewrite of the expat detection in apr-util to fix lingering 
problems like PR 28205 caused by attempting to detect presence of 
libraries/headers using test -f:

...

This works on my Solaris 10 x86 32 bits
The patch does not seem to solve the 64-bit issue, as I see 'lib' is 
assumed to be the library path, am I missing something?


Thanks for testing it out.  How did you configure, and how did it fail? 
(config.log preferred)




I haven't got a chance to test on a 64-bits machine with lib64 folder. I 
was simply reading the patch and wondering how it addresses issue 28205.


http://issues.apache.org/bugzilla/show_bug.cgi?id=28205.


joe

Latest apr_thread_pool patch is submitted as bug 41061

[Henry Jen]

Hi,

To make sure the patch won't get lost, I filed an issue for it. :-)

http://issues.apache.org/bugzilla/show_bug.cgi?id=41061

Cheers,
Henry

Re: [PATCH] rewrite expat detection

[Henry Jen]

Joe Orton wrote:
This is a rewrite of the expat detection in apr-util to fix lingering 
problems like PR 28205 caused by attempting to detect presence of 
libraries/headers using test -f:


- jumps through hoops to retain support for older versions of expat; 
could be dumped at some point but probably not in 1.x

- does still explicitly check in /usr/local
- probably breaks the VPATH build with bundled expat but I'll fix that
before committing anything
- aiming for trunk not the 1.2.x branch!

This could do with some testing on some non-Linuxy Unixes.




Hi Joe,

This works on my Solaris 10 x86 32 bits
The patch does not seem to solve the 64-bit issue, as I see 'lib' is 
assumed to be the library path, am I missing something?


Cheers,
Henry


Index: build/apu-conf.m4
===
--- build/apu-conf.m4   (revision 474791)
+++ build/apu-conf.m4   (working copy)
@@ -46,108 +46,86 @@
   AC_SUBST(APR_BUILD_DIR)
 ])
 
-

 dnl
-dnl APU_TEST_EXPAT(directory): test if Expat is located in the specified dir
+dnl APU_TRY_EXPAT_LINK(
+dnl  test-message, cache-var-name, hdrs, libs,
+dnl  [actions-on-success], [actions-on-failure])
+dnl 
+dnl Tests linking against expat with libraries 'libs' and includes

+dnl 'hdrs', passing message + cache-var-name to AC_CACHE_CHECK.
+dnl On success, sets $expat_libs to libs, sets $apu_have_expat to 1, 
+dnl and runs actions-on-success; on failure runs actions-on-failure.

 dnl
-dnl if present: sets expat_include_dir, expat_libs, possibly expat_old
+AC_DEFUN([APU_TRY_EXPAT_LINK], [
+AC_CACHE_CHECK([$1], [$2], [
+  apu_expat_LIBS=$LIBS
+  LIBS=$LIBS $4
+  AC_TRY_LINK([#include stdlib.h
+#include $3], [XML_ParserCreate(NULL);],
+[$2=yes], [$2=no])
+  LIBS=$apu_expat_LIBS
+])
+
+if test $[$2] = yes; then
+   AC_DEFINE([HAVE_]translit([$3], [a-z./], [A-Z__]), 1,
+ [Define if $3 is available])
+   apu_expat_libs=$4
+   apu_has_expat=1
+   $5
+else
+   apu_has_expat=0
+   $6
+fi
+])
+
 dnl
-AC_DEFUN([APU_TEST_EXPAT], [
-  AC_MSG_CHECKING(for Expat in ifelse($2,,$1,$2))
+dnl APU_SYSTEM_EXPAT: tests for a system expat installation
+dnl If present, sets $apu_has_expat to 1
+dnl
+AC_DEFUN([APU_SYSTEM_EXPAT], [
+ 
+  APU_TRY_EXPAT_LINK([Expat 1.95.x], apu_cv_expat_system, 
+[expat.h], [-lexpat])
 
-  expat_libtool=

+  if test $apu_has_expat = 0; then
+APU_TRY_EXPAT_LINK([old Debian-packaged expat], apu_cv_expat_debian,
+   [xmltok/xmlparse.h], [-lxmlparse -lxmltok])
+  fi
 
-  if test -r $1/lib/expat.h.in; then

-dnl Expat 1.95.* distribution
-expat_include_dir=$1/lib
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.la; then
-dnl Expat 1.95.* installation (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib64/libexpat.la; then
-dnl Expat 1.95.* installation on certain 64-bit platforms (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib64
-expat_libs=-lexpat
-expat_libtool=$1/lib64/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.a; then
-dnl Expat 1.95.* static installation (without libtool)
-dnl FreeBSD textproc/expat2
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.so; then
-dnl Expat 1.95.* shared installation (without libtool)
-dnl Solaris 10 /usr/sfw
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-  elif test -r $1/xmlparse.h; then
-dnl maybe an expat-lite. use this dir for both includes and libs
-expat_include_dir=$1
-expat_ldflags=-L$1
-expat_libs=-lexpat
-expat_libtool=$1/libexpat.la
-expat_old=yes
-  elif test -r $1/include/xmlparse.h -a \
-   -r $1/lib/libexpat.a; then
-dnl previously installed expat
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_old=yes
-  elif test -r $1/include/xml/xmlparse.h -a \
-   -r $1/lib/xml/libexpat.a; then
-dnl previously installed expat
-expat_include_dir=$1/include/xml
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_old=yes
-  elif test -r $1/include/xmltok/xmlparse.h; then
-dnl Debian distribution
-expat_include_dir=$1/include/xmltok
-expat_ldflags=-L$1/lib
-expat_libs=-lxmlparse -lxmltok
-expat_old=yes
-  elif test -r $1/include/xml/xmlparse.h -a \
-   -r $1/lib/libexpat.a; then
-dnl FreeBSD textproc/expat package
-expat_include_dir=$1/include/xml
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_old=yes
-  elif test -r $1/xmlparse/xmlparse.h; then
-dnl Expat 1.0 or 1.1 source directory
-expat_include_dir=$1/xmlparse
-

Re: Tagging a tarball friday, 1.2.x, perhaps 0.9.x

[Henry Jen]

William A. Rowe, Jr. wrote:

I'm primarilly interested in making a 1.2 tarball to vote on in 48
hours because we have fixed a number of bugs.  Although there are
still a few more in-queue (and I welcome all help to get these fixed
in the branch), I'm mostly thinking that a bugfix stable release would
be good - before I commit any code to support 'new things' in HFS (OS/X),
that might introduce new breakage or build oddities for a subset of
the users.

I can roll 0.9 at the same time, if there is sufficient interest.  But
wouldn't mind 'skipping' a cycle or two - and focusing on the bug cleanups
added for 1.2 - in preparation for 1.3.0.



Bug 40892 for libexpat detection and 40893 for apr_brigade_length were 
recently filed but had been proposed and discussed on [EMAIL PROTECTED] for some time.


IIRC, questions to those patches are explained, is there any other 
concern for those 2 relative small patches?


http://issues.apache.org/bugzilla/show_bug.cgi?id=40892
http://issues.apache.org/bugzilla/show_bug.cgi?id=40893

Cheers,
Henry

Re: [PATCH] apr_threadpool

[Henry Jen]

Henry Jen wrote:

Hi,

Attached please find the patch for thread pool implementation, looking 
forward to see it get committed.




I just realized that I sent the wrong patch, which did not drop the 
copyright notice. Attached is the correct patch. :-)


Just want to make sure the consensus is the code is ready for commit and 
is now simply waiting some committer's love.


Cheers,
Henry



Index: aprutil.dsp
===
--- aprutil.dsp	(revision 453014)
+++ aprutil.dsp	(working copy)
@@ -240,6 +240,10 @@
 # PROP Default_Filter 
 # Begin Source File
 
+SOURCE=.\misc\apr_thread_pool.c
+# End Source File
+# Begin Source File
+
 SOURCE=.\misc\apr_date.c
 # End Source File
 # Begin Source File
@@ -512,6 +516,10 @@
 # End Source File
 # Begin Source File
 
+SOURCE=.\include\apr_thread_pool.h
+# End Source File
+# Begin Source File
+
 SOURCE=.\include\apr_date.h
 # End Source File
 # Begin Source File
Index: include/apr_thread_pool.h
===
--- include/apr_thread_pool.h	(revision 0)
+++ include/apr_thread_pool.h	(revision 0)
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed
+ * with this work for additional information regarding copyright
+ * ownership.  The ASF licenses this file to you under the Apache
+ * License, Version 2.0 (the License); you may not use this file
+ * except in compliance with the License.  You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied.  See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy and the
+ * number of tasks in the queue is higher than the adjustable threshold, the
+ * pool will try to create a new thread to serve the task if the maximum number
+ * has not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C
+{
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct apr_thread_pool apr_thread_pool_t;
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Create a thread pool
+ * @param me A pointer points to the pointer receives the created
+ * apr_thread_pool object. The returned value will be NULL if failed to create
+ * the thread pool.
+ * @param init_threads The number of threads to be created initially, the number
+ * will also be used as the initial value for maximum number of idle threads. 
+ * @param max_threads The maximum number of threads that can be created
+ * @param pool The pool to use
+ * @return APR_SUCCESS if the thread pool was created successfully. Otherwise,
+ * the error code.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
+ apr_size_t init_threads,
+ apr_size_t max_threads,
+ apr_pool_t * pool);
+
+/**
+ * Destroy the thread pool and stop all the threads
+ * @return APR_SUCCESS if all threads are stopped.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t * me);
+
+/**
+ * Schedule a task to the bottom of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @param owner Owner of this task

Re: Dialog - NOTICEs at APR

[Henry Jen]

Colm MacCarthaigh wrote:

On Wed, Sep 27, 2006 at 05:56:56PM -0400, Garrett Rooney wrote:

If Sun or Covalent gives a damn about getting properly credited then
they can tell their committers to use their @themothership.com address
on the list.


When people gain commit, with emphasis on the word people there,
we generally start refering to them by name only. 


As a meritocracy, we give credit to the individuals who write code or
docs or whatever, and that's what names in CHANGES are really about.
We don't give a crap who they work for, or if they change jobs, or
e-mail addresses, the credit is personal to them.

Acknowledging a copyright is a different thing IMO. Personal
contributors can own their own copyright, or frequently, it may be owned
by their employer. If a copyright owner wants the acknowledgement
they are rightly entitled there is nothing we can do about it. Basic
copyright law, and the ASL 2.0 4.c (which is how the work is licensed
to us) definitely apply.

It's a good idea that copyright owners get explicit notices if they want
them, after all if someone wants to re-license that content, or if they
suspect an infringement, they need to contact the primary copyright
owner. 


We can choose to accept he patch with those terms, or to not accept it,
but we absolutely cannot over-ride the copyright owners wishes. 


Of course, we can come up with a useful suggestion, hopefully acceptable
to all. but it's worth noting that right now ASL 2.0 4.d basically
suggests that the NOTICES file is the correct place for notices of this
nature. 




This is a very good comment and says everything I have to say for this 
topic, I chose to keep silence in purpose before we can settle down the 
discussion. :-)


Glad to tell you that our legal department is sane enough and agreed we 
can just drop the notice as option 1.a. Hopefully this remove the hurdle 
and can get the code committed.


Should I submit the patch once again or this email is enough to show you 
we agree to drop the copyright notice?


This is taking longer than what I had expected, as from very beginning 
we wanted to contribute the code. It's me that not being familiar with 
both Sun's process and Apache's process. It feels good all the 
legal/process issues seems to be completed. :-)


Cheers,
Henry

[PATCH] apr_threadpool

[Henry Jen]

Hi,

Attached please find the patch for thread pool implementation, looking 
forward to see it get committed.


Cheers,
Henry



Index: aprutil.dsp
===
--- aprutil.dsp	(revision 433720)
+++ aprutil.dsp	(working copy)
@@ -240,6 +240,10 @@
 # PROP Default_Filter 
 # Begin Source File
 
+SOURCE=.\misc\apr_thread_pool.c
+# End Source File
+# Begin Source File
+
 SOURCE=.\misc\apr_date.c
 # End Source File
 # Begin Source File
@@ -512,6 +516,10 @@
 # End Source File
 # Begin Source File
 
+SOURCE=.\include\apr_thread_pool.h
+# End Source File
+# Begin Source File
+
 SOURCE=.\include\apr_date.h
 # End Source File
 # Begin Source File
Index: include/apr_thread_pool.h
===
--- include/apr_thread_pool.h	(revision 0)
+++ include/apr_thread_pool.h	(revision 0)
@@ -0,0 +1,237 @@
+/* Copyright 2006 Sun Microsystems, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License);
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy and the
+ * number of tasks in the queue is higher than the adjustable threshold, the
+ * pool will try to create a new thread to serve the task if the maximum number
+ * has not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C
+{
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct apr_thread_pool apr_thread_pool_t;
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Create a thread pool
+ * @param me A pointer points to the pointer receives the created
+ * apr_thread_pool object. The returned value will be NULL if failed to create
+ * the thread pool.
+ * @param init_threads The number of threads to be created initially, the number
+ * will also be used as the initial value for maximum number of idle threads. 
+ * @param max_threads The maximum number of threads that can be created
+ * @param pool The pool to use
+ * @return APR_SUCCESS if the thread pool was created successfully. Otherwise,
+ * the error code.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
+ apr_size_t init_threads,
+ apr_size_t max_threads,
+ apr_pool_t * pool);
+
+/**
+ * Destroy the thread pool and stop all the threads
+ * @return APR_SUCCESS if all threads are stopped.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t * me);
+
+/**
+ * Schedule a task to the bottom of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @param owner Owner of this task.
+ * @return APR_SUCCESS if the task had been scheduled successfully
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t * me,
+   apr_thread_start_t func,
+   void *param,
+   apr_byte_t priority,
+   void *owner);
+/**
+ * Schedule a task to be run after a delay
+ * @param me 

Re: apr_brigade_length to return length as is instead of undefined

[Henry Jen]

Garrett Rooney wrote:

On 9/23/06, Henry Jen [EMAIL PROTECTED] wrote:

Hi,

apr_brigade_length with readall set to 1 will try to read from a bucket
with length -1 to determine the value.

Currently, if apr_bucket_read with the bucket of length -1 returns other
than APR_SUCCESS, the length returned is undefined.

In case of a socket bucket with a nonblocking socket, the return status
could be EAGAIN and the length is still useful to determine value
available at the moment. A use case, for example, is waiting for a
certain amount of data to be available.

I would propose to return the value up-to-date is better than undefined.
The application can decide whether the value is meaningful with the
returned status. Does this make sense?

Attached is a simple path specifying the length.


Is total going to be the correct value in that case?  I know in
apr_file_read you can get stuff in *len even if an error has occurred,
can that occur with apr_bucket_read?  If so, it seems like you'd want
*length = total + *len, or something like that.



It may not be accurate, but it is what you can get from the brigade for 
sure.


In the socket example, you are not likely to get an length back when 
there is an error. Even you do have a length, the data read was 
discarded so that it won't be in the brigade.


Given that, I think not adding the *len value might be a better solution.

Cheers,
Henry

Re: [PATCH] apr_threadpool

[Henry Jen]

Hi,

Attached is an updated patch, which include following changes since last 
one:


1. The check for current thread not to be the owner include a hack for 
the Win32 bug where apr_os_thread_current is returning a HANDLE instead 
of a pointer to HANDLE as apr_os_thread_get does.


2. Remove the copyright notice and apply source header to be conforming 
with Source File Headers for Code Developed at the ASF section in [1]. 
Our legal would like to see the copyright notice in NOTICE file per 
option 1.b. But I am not sure how APR would like to deal with it.


Cheers,
Henry

[1] http://www.apache.org/legal/src-headers.html

William A. Rowe, Jr. wrote:

I'm +1 on adopting this in trunk so that the developers can spend some
time with the API, and we can begin duking out the details.

I suspect it could use a bit of tweaking, but it's easier to do that in
svn - the email thread has already yielded a cleaner API.

Henry Jen wrote:

Hi,

Attached please find the latest patch to support thread_pool(the last
one had a bug and cannot be merged cleanly), which has two enhancement
from earlier patch:

1. Ownership support: Now when submit a task to the thread pool, an
owner identity can be specified. Which can be used to remove all tasks
belongs to that owner afterwards.

2. Timer: It is possible to schedule a task to be executed after a
certain time interval. By doing another schedule within a task, that is
basically a timer. Noted that scheduled tasks are with highest priority,
and due tasks are always picked up first.

Comments are always welcome, and hopefully soon this can be committed.

Cheers,
Henry

Index: aprutil.dsp
===
--- aprutil.dsp	(revision 449343)
+++ aprutil.dsp	(working copy)
@@ -240,6 +240,10 @@
 # PROP Default_Filter 
 # Begin Source File
 
+SOURCE=.\misc\apr_thread_pool.c
+# End Source File
+# Begin Source File
+
 SOURCE=.\misc\apr_date.c
 # End Source File
 # Begin Source File
@@ -512,6 +516,10 @@
 # End Source File
 # Begin Source File
 
+SOURCE=.\include\apr_thread_pool.h
+# End Source File
+# Begin Source File
+
 SOURCE=.\include\apr_date.h
 # End Source File
 # Begin Source File
Index: include/apr_thread_pool.h
===
--- include/apr_thread_pool.h	(revision 0)
+++ include/apr_thread_pool.h	(revision 0)
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed
+ * with this work for additional information regarding copyright
+ * ownership.  The ASF licenses this file to you under the Apache
+ * License, Version 2.0 (the License); you may not use this file
+ * except in compliance with the License.  You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied.  See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy and the
+ * number of tasks in the queue is higher than the adjustable threshold, the
+ * pool will try to create a new thread to serve the task if the maximum number
+ * has not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C
+{
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct apr_thread_pool apr_thread_pool_t;
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Create a thread pool
+ * @param me A pointer points to the pointer receives the created

apr_brigade_length to return length as is instead of undefined

[Henry Jen]

Hi,

apr_brigade_length with readall set to 1 will try to read from a bucket 
with length -1 to determine the value.


Currently, if apr_bucket_read with the bucket of length -1 returns other 
than APR_SUCCESS, the length returned is undefined.


In case of a socket bucket with a nonblocking socket, the return status 
could be EAGAIN and the length is still useful to determine value 
available at the moment. A use case, for example, is waiting for a 
certain amount of data to be available.


I would propose to return the value up-to-date is better than undefined. 
The application can decide whether the value is meaningful with the 
returned status. Does this make sense?


Attached is a simple path specifying the length.

Cheers,
Henry
Index: buckets/apr_brigade.c
===
--- buckets/apr_brigade.c	(revision 449343)
+++ buckets/apr_brigade.c	(working copy)
@@ -182,6 +182,7 @@
 
 if ((status = apr_bucket_read(bkt, ignore, len,
   APR_BLOCK_READ)) != APR_SUCCESS) {
+		*length = total;
 return status;
 }
 }

Re: svn commit: r438795 - /apr/apr-util/trunk/build/apu-conf.m4

[Henry Jen]

[EMAIL PROTECTED] wrote:

Author: jerenkrantz
Date: Wed Aug 30 22:04:40 2006
New Revision: 438795

URL: http://svn.apache.org/viewvc?rev=438795view=rev
Log:
* build/apu-conf.m4
  (APU_TEST_EXPAT): Support Solaris /usr/sfw installs of Expat.

Modified:
apr/apr-util/trunk/build/apu-conf.m4

Modified: apr/apr-util/trunk/build/apu-conf.m4
URL: 
http://svn.apache.org/viewvc/apr/apr-util/trunk/build/apu-conf.m4?rev=438795r1=438794r2=438795view=diff
==
--- apr/apr-util/trunk/build/apu-conf.m4 (original)
+++ apr/apr-util/trunk/build/apu-conf.m4 Wed Aug 30 22:04:40 2006
@@ -79,8 +79,15 @@
 expat_libtool=$1/lib64/libexpat.la
   elif test -r $1/include/expat.h -a \
 -r $1/lib/libexpat.a; then
-dnl Expat 1.95.* installation (without libtool)
+dnl Expat 1.95.* static installation (without libtool)
 dnl FreeBSD textproc/expat2
+expat_include_dir=$1/include
+expat_ldflags=-L$1/lib
+expat_libs=-lexpat
+  elif test -r $1/include/expat.h -a \
+-r $1/lib/libexpat.so; then
+dnl Expat 1.95.* shared installation (without libtool)
+dnl Solaris 10 /usr/sfw
 expat_include_dir=$1/include
 expat_ldflags=-L$1/lib
 expat_libs=-lexpat





This is another hardcode case, is there a possibility to consider a 
patch I submitted[1] which use AC_CHECK_LIB to cover all cases with 
$1/include/expat.h available?


An updated patch is attached.

[1] 
http://mail-archives.apache.org/mod_mbox/apr-dev/200608.mbox/[EMAIL PROTECTED]


Cheers,
Henry

Index: build/apu-conf.m4
===
--- build/apu-conf.m4	(revision 442408)
+++ build/apu-conf.m4	(working copy)
@@ -63,34 +63,24 @@
 expat_ldflags=-L$1/lib
 expat_libs=-lexpat
 expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.la; then
-dnl Expat 1.95.* installation (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib64/libexpat.la; then
-dnl Expat 1.95.* installation on certain 64-bit platforms (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib64
-expat_libs=-lexpat
-expat_libtool=$1/lib64/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.a; then
-dnl Expat 1.95.* static installation (without libtool)
-dnl FreeBSD textproc/expat2
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.so; then
-dnl Expat 1.95.* shared installation (without libtool)
-dnl Solaris 10 /usr/sfw
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
+  elif test -r $1/include/expat.h; then
+old_ldflags=$LDFLAGS
+old_cflags=$CFLAGS
+for d in $1/lib $1/libdir ; do
+  CFLAGS=$old_cflags -I$1/include
+  LDFLAGS=$old_ldflags -L$d
+  AC_CHECK_LIB(expat, XML_ParserCreate, [
+expat_include_dir=$1/include
+expat_libs=-lexpat
+expat_ldflags=-L$d
+if test -r $d/libexpat.la; then
+  expat_libtool=$d/libexpat.la
+fi
+break
+  ], [])
+done
+CFLAGS=$old_cflags
+LDFLAGS=$old_ldflags
   elif test -r $1/xmlparse.h; then
 dnl maybe an expat-lite. use this dir for both includes and libs
 expat_include_dir=$1

Re: apr_pollset_poll does not return when socket closed

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:

Justin Erenkrantz wrote:

In short, there's no way to portably detect the socket is closed via a
poll()-like mechanism.  It seems like everyone keeps running into
this.  Apparently, the only way to know if it is closed is to read()
from it - which really sucks.  -- justin

Thanks for the pointer. :-)


Actually it should return from poll() on close of the 'foreign' end of the
socket.  Closing the 'local' end under the poll() is undefined.


As I read it, seems like I could expect it to return from poll, just the
event can be different, is that correct?

The result I see is that apr_pollset_poll is not returning.


Right, you can't close the poll'ed end of the socket underneath the poll(),
it's not portable, it's not good design.  Heck, if it caused the app to fault
I would just laugh at you ;-)



Hehe, cannot tell it is not defined behavior from document, and have no 
idea on portability of this. Just a feeling that a socket closed should 
be worth attention. :-)



My guess is that because there is no connection at all because what the
code does is simply listening on the socket.

If that's not possible, how do I remove the socket from the pollset in a
thread-safe way on platforms using select like Win32?


Questions; do you really need to?  Or can you wait till the poll exits on
real activity?  Do you have both ends of the socket, and can you close the
other end (the fair-game side)?




If you are listening on a socket, and poll without timeout(blocking 
infinitely until an event), and there is no connection, what option do 
you have?


I am now using a timeout just to avoid this issue. not that timeout is a 
bad idea, just think if handling traffic is the only thing to do, I 
would rather like it to block.


Cheers,
Henry

Win32 apr_os_thread_get and apr_os_thread_put has mismatch type

[Henry Jen]

Hi,

On Windows apr_os_threads_t is defined as HANDLE, but in 
apr_os_thread_get and apr_os_thread_put implementation for Win32, it 
mis-use apr_os_thread_t* as HANDLE(thd-td is a HANDLE).


Attached is a patch fix that issue, could someone please review it?

Cheers,
Henry
Index: threadproc/win32/thread.c
===
--- threadproc/win32/thread.c	(revision 440895)
+++ threadproc/win32/thread.c	(working copy)
@@ -234,7 +234,7 @@
 if (thd == NULL) {
 return APR_ENOTHREAD;
 }
-*thethd = thd-td;
+*thethd = thd-td;
 return APR_SUCCESS;
 }
 
@@ -249,7 +249,7 @@
 (*thd) = (apr_thread_t *)apr_palloc(pool, sizeof(apr_thread_t));
 (*thd)-pool = pool;
 }
-(*thd)-td = thethd;
+(*thd)-td = *thethd;
 return APR_SUCCESS;
 }
 

Re: Win32 apr_os_thread_get and apr_os_thread_put has mismatch type

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:

-*thethd = thd-td;
+*thethd = thd-td;


-1 - I don't know if you noticed but you just returned to the caller
the address of the HANDLE in the thd object, meaning that it won't
outlive the destruction of the thd.




Good point. What's your suggestion?

The patch is bad, but it won't change the fact this is a bug.

Unfortunately, I don't see a easy way out top off my head.

Cheers,
Henry

Re: apr_pollset_poll does not return when socket closed

[Henry Jen]

Justin Erenkrantz wrote:

On 8/31/06, Henry Jen [EMAIL PROTECTED] wrote:

Haven't get a chance to test on other platform, but wondering if this is
reasonable to expect. If it is, what do I miss in the testing code?


You should read:

http://mail-archives.apache.org/mod_mbox/apr-dev/200608.mbox/[EMAIL PROTECTED] 



In short, there's no way to portably detect the socket is closed via a
poll()-like mechanism.  It seems like everyone keeps running into
this.  Apparently, the only way to know if it is closed is to read()
from it - which really sucks.  -- justin



Thanks for the pointer. :-)

As I read it, seems like I could expect it to return from poll, just the 
event can be different, is that correct?


The result I see is that apr_pollset_poll is not returning.

My guess is that because there is no connection at all because what the 
code does is simply listening on the socket.


If that's not possible, how do I remove the socket from the pollset in a 
thread-safe way on platforms using select like Win32?


Cheers,
Henry

apr_pollset_poll does not return when socket closed

[Henry Jen]

Hi,

I was expecting apr_pollset_poll to return from blocking if the socket 
was closed, but this does not seems to be the case on OpenSolaris(Neveda 
build 35).


Haven't get a chance to test on other platform, but wondering if this is 
reasonable to expect. If it is, what do I miss in the testing code?


Attached please find the test program reveals the problem, basically 
what it does is to listen on a socket, and start a thread to poll on 
that socket. Wait a while to close the socket and join on the polling 
thread.


Any ideas are welcome, thank you in advance. :-)

Cheers,
Henry
#include apr_poll.h
#include apr_thread_proc.h
#include apr_network_io.h

static int running = 0;

static void* APR_THREAD_FUNC do_poll(apr_thread_t * thd, void * arg)
{
apr_pollset_t * pollset = arg;
apr_status_t rv;
apr_int32_t num;
const apr_pollfd_t * fds;
apr_int32_t ev;

while (running) {
printf(Waiting apr_pollset_poll ...\n);
rv = apr_pollset_poll(pollset, -1L, num, fds);
if (APR_SUCCESS != rv) {
printf(apr_pollset_poll failed with status %d\n, rv);
return NULL;
}
printf(Came back from apr_pollset_poll.\n);

while (num  0) {
num --;
ev = fds-rtnevents;
printf(APR_POLLIN %d APR_POLLPRI %d APR_POLLERR %d 
   APR_POLLHUP %d APR_POLLNVAL %d\n, ev  APR_POLLIN ? 1:0, 
   ev  APR_POLLPRI ? 1:0, ev  APR_POLLERR  ? 1:0, 
   ev  APR_POLLHUP ? 1:0, ev  APR_POLLNVAL ? 1:0);
fds++;
}
}
return NULL;
}

apr_status_t 
socket_listen(apr_socket_t ** me, const char * addr, apr_port_t port, 
  apr_int32_t backlog, apr_pool_t * p)
{
apr_status_t rv;
apr_sockaddr_t * sa;
char msg[256];

rv = apr_sockaddr_info_get(sa, addr, APR_UNSPEC, port, 0, p);
if (APR_SUCCESS != rv) {
apr_strerror(rv, msg, sizeof(msg));
printf(Socket addr info failed : %s\n, msg);
*me = NULL;
return rv;
}

rv = apr_socket_create(me, sa-family, SOCK_STREAM, APR_PROTO_TCP, p);
if (APR_SUCCESS != rv) {
apr_strerror(rv, msg, sizeof(msg));
printf(Failed create server socket : %s\n, msg);
*me = NULL;
return rv;
}

/* REUSEADDR: this may help in case of the port is in TIME_WAIT */
/*
rv = apr_socket_opt_set(*me, APR_SO_REUSEADDR, 1);
if (APR_SUCCESS != rv) {
apr_strerror(rv, msg, sizeof(msg));
printf(Failed setting options : %s\n, msg);
}
*/
/* bind */
rv = apr_socket_bind(*me, sa);
if (APR_SUCCESS != rv) {
apr_socket_close(*me);
apr_strerror(rv, msg, sizeof(msg));
printf(Failed to bind socket : %s\n, msg);
*me = NULL;
return rv;
}

/* listen */
rv = apr_socket_listen(*me, backlog);
if (APR_SUCCESS != rv) {
apr_socket_close(*me);
apr_strerror(rv, msg, sizeof(msg));
printf(Failed to listen on socket : %s\n, msg);
*me = NULL;
return rv;
}

return APR_SUCCESS;
}

int main(int argc, char **argv)
{
apr_pool_t *p;
apr_pollset_t *pollset;
apr_status_t rv;
apr_socket_t *s;
apr_pollfd_t fd;
apr_thread_t *thd;

apr_initialize();
rv = apr_pool_create(p, NULL);
rv = apr_pollset_create(pollset, 10, p, APR_POLLSET_THREADSAFE);
if (APR_SUCCESS != rv) {
return rv;
}

rv = socket_listen(s, 127.0.0.1, 9701, 10, p);
if (APR_SUCCESS != rv) {
return rv;
}

fd.p = p;
fd.desc_type = APR_POLL_SOCKET;
fd.desc.s = s;
fd.reqevents = APR_POLLIN | APR_POLLPRI | APR_POLLERR | APR_POLLHUP | 
   APR_POLLNVAL;

rv = apr_pollset_add(pollset, fd);
if (APR_SUCCESS != rv) {
printf(Failed to add pollset\n);
return rv;
}

running = 1;
rv = apr_thread_create(thd, NULL, do_poll, pollset, p);
if (APR_SUCCESS != rv) {
printf(Failed to create thread\n);
return rv;
}

printf(Wait for 60 seconds ...\n);
apr_sleep(60L * 100L);
printf(Closing socket ...\n);

running = 0;
apr_socket_shutdown(s, APR_SHUTDOWN_READWRITE);
apr_socket_close(s);

printf(Waiting polling thread to stop ...\n);
apr_thread_join(rv, thd);
apr_pool_destroy(p);
apr_terminate();
return 0;
}

/* vim: set et sw=4 ts=4 tw=80: */

[PATCH] check libexpat when there is no libexpat.la or libexpat.a[for OpenSolaris]

[Henry Jen]

Hi,

Attached patch allows to build apr-util with system provided libexpat on 
OpenSolaris.


On the machine, there is a expat.h and libexpat.so.* in /usr/sfw/include 
and /usr/sfw/lib. But there is no .a or .la file.


It should not be required to have a .la file, not to mention a .a file. 
.a is, afaik, an archive file for static link.


Attached patch use AC_CHECK_LIB to ensure a library can be found, and it 
uses a for loop to look into potential folders, currently I put lib and 
lib64 in there.


Cheers,
Henry


Index: build/apu-conf.m4
===
--- build/apu-conf.m4	(revision 433720)
+++ build/apu-conf.m4	(working copy)
@@ -63,27 +63,24 @@
 expat_ldflags=-L$1/lib
 expat_libs=-lexpat
 expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.la; then
-dnl Expat 1.95.* installation (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib64/libexpat.la; then
-dnl Expat 1.95.* installation on certain 64-bit platforms (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib64
-expat_libs=-lexpat
-expat_libtool=$1/lib64/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.a; then
-dnl Expat 1.95.* installation (without libtool)
-dnl FreeBSD textproc/expat2
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
+  elif test -r $1/include/expat.h; then
+old_ldflags=$LDFLAGS
+old_cflags=$CFLAGS
+for d in $1/lib $1/lib64 ; do
+  CFLAGS=$old_cflags -I$1/include
+  LDFLAGS=$old_ldflags -L$d
+  AC_CHECK_LIB(expat, XML_ParserCreate, [
+expat_include_dir=$1/include
+expat_libs=-lexpat
+expat_ldflags=-L$d
+if test -r $d/libexpat.la; then
+  expat_libtool=$d/libexpat.la
+fi
+break
+  ], [])
+done
+CFLAGS=$old_cflags
+LDFLAGS=$old_ldflags
   elif test -r $1/xmlparse.h; then
 dnl maybe an expat-lite. use this dir for both includes and libs
 expat_include_dir=$1

[PATCH] apr_threadpool

[Henry Jen]

Hi,

Attached please find the latest patch to support thread_pool(the last 
one had a bug and cannot be merged cleanly), which has two enhancement 
from earlier patch:


1. Ownership support: Now when submit a task to the thread pool, an 
owner identity can be specified. Which can be used to remove all tasks 
belongs to that owner afterwards.


2. Timer: It is possible to schedule a task to be executed after a 
certain time interval. By doing another schedule within a task, that is 
basically a timer. Noted that scheduled tasks are with highest priority, 
and due tasks are always picked up first.


Comments are always welcome, and hopefully soon this can be committed.

Cheers,
Henry


Index: aprutil.dsp
===
--- aprutil.dsp	(revision 433720)
+++ aprutil.dsp	(working copy)
@@ -240,6 +240,10 @@
 # PROP Default_Filter 
 # Begin Source File
 
+SOURCE=.\misc\apr_thread_pool.c
+# End Source File
+# Begin Source File
+
 SOURCE=.\misc\apr_date.c
 # End Source File
 # Begin Source File
@@ -512,6 +516,10 @@
 # End Source File
 # Begin Source File
 
+SOURCE=.\include\apr_thread_pool.h
+# End Source File
+# Begin Source File
+
 SOURCE=.\include\apr_date.h
 # End Source File
 # Begin Source File
Index: include/apr_thread_pool.h
===
--- include/apr_thread_pool.h	(revision 0)
+++ include/apr_thread_pool.h	(revision 0)
@@ -0,0 +1,237 @@
+/* Copyright 2006 Sun Microsystems, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License);
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy and the
+ * number of tasks in the queue is higher than the adjustable threshold, the
+ * pool will try to create a new thread to serve the task if the maximum number
+ * has not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C
+{
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct apr_thread_pool apr_thread_pool_t;
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Create a thread pool
+ * @param me A pointer points to the pointer receives the created
+ * apr_thread_pool object. The returned value will be NULL if failed to create
+ * the thread pool.
+ * @param init_threads The number of threads to be created initially, the number
+ * will also be used as the initial value for maximum number of idle threads. 
+ * @param max_threads The maximum number of threads that can be created
+ * @param pool The pool to use
+ * @return APR_SUCCESS if the thread pool was created successfully. Otherwise,
+ * the error code.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
+ apr_size_t init_threads,
+ apr_size_t max_threads,
+ apr_pool_t * pool);
+
+/**
+ * Destroy the thread pool and stop all the threads
+ * @return APR_SUCCESS if all threads are stopped.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t * me);
+
+/**
+ * Schedule a task to the bottom of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task 

[PATCH] thread pool implementation again with owner and timer support

[Henry Jen]

Hi,

Attached please find the latest patch to support thread_pool, which has 
two enhancement from previous patch:


1. Ownership support: Now when submit a task to the thread pool, an 
owner identity can be specified. Which can be used to remove all tasks 
belongs to that owner afterwards.


2. Timer: It is possible to schedule a task to be executed after a 
certain time interval. By doing another schedule within a task, that is 
basically a timer. Noted that scheduled tasks are with highest priority, 
and due tasks are always picked up first.


Comments are always welcome, and hopefully soon this can be committed.

Cheers,
Henry
Index: aprutil.dsp
===
--- aprutil.dsp	(revision 430516)
+++ aprutil.dsp	(working copy)
@@ -240,6 +240,10 @@
 # PROP Default_Filter 
 # Begin Source File
 
+SOURCE=.\misc\apr_thread_pool.c
+# End Source File
+# Begin Source File
+
 SOURCE=.\misc\apr_date.c
 # End Source File
 # Begin Source File
@@ -512,6 +516,10 @@
 # End Source File
 # Begin Source File
 
+SOURCE=.\include\apr_thread_pool.h
+# End Source File
+# Begin Source File
+
 SOURCE=.\include\apr_date.h
 # End Source File
 # Begin Source File
--- /dev/null	Fri Aug 11 14:27:11 2006
+++ include/apr_thread_pool.h	Fri Aug 11 13:55:33 2006
@@ -1,0 +1,237 @@
+/* Copyright 2006 Sun Microsystems, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License);
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy and the
+ * number of tasks in the queue is higher than the adjustable threshold, the
+ * pool will try to create a new thread to serve the task if the maximum number
+ * has not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C
+{
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct apr_thread_pool apr_thread_pool_t;
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Create a thread pool
+ * @param me A pointer points to the pointer receives the created
+ * apr_thread_pool object. The returned value will be NULL if failed to create
+ * the thread pool.
+ * @param init_threads The number of threads to be created initially, the number
+ * will also be used as the initial value for maximum number of idle threads. 
+ * @param max_threads The maximum number of threads that can be created
+ * @param pool The pool to use
+ * @return APR_SUCCESS if the thread pool was created successfully. Otherwise,
+ * the error code.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
+ apr_size_t init_threads,
+ apr_size_t max_threads,
+ apr_pool_t * pool);
+
+/**
+ * Destroy the thread pool and stop all the threads
+ * @return APR_SUCCESS if all threads are stopped.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t * me);
+
+/**
+ * Schedule a task to the bottom of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @param owner Owner of this task.
+ * @return APR_SUCCESS if the task had been scheduled 

Re: Implementation apr_thread_pool

[Henry Jen]

Marco Spinetti wrote:

Hi,
I'm very curious about the status of implementation of apr_thread_pool.
Is it stable or not?
Where can I download it?
How can I build and try it?
I would like to create an application based on it.




Hi Marco,

I prepared a patch a while ago, and those are used in our project right now.

You can download the current implementation of apr_thread_pool.[ch] at
http://jxta-c.jxta.org/source/browse/jxta-c/src/jpr/

We are looking to add owner and timer support in a couple days, the 
patch is almost ready and is under internal review.


Cheers,
Henry

Re: APR threads consume a large amount of virtual memory

[Henry Jen]

Ivan Ristic wrote:

I have noticed that my multithreaded APR program consumes a *very*
large amount of virtual memory per thread and I can't think of a
reason why. I am running Debian 3.1  (latest kernel 2.6.8-3-686) and I
tried the test program below with both the APR 0.9.x that comes with
Debian and with the latest APR 1.2.x version.

In both cases I end up with around 800 MB of virtual RAM for 100 threads:
20385 ivanr 16   0  802m  956 1800 S  0.0  0.4   0:00.02 test

Am I doing something wrong or is this a bug? Any help is greatly 
appreciated!


void* APR_THREAD_FUNC thread_worker(apr_thread_t *thread, void *data) {
   apr_thread_exit(thread, 0);
}

int main(int argc, const char * const argv[]) {
   apr_pool_t *pool;
   int i;

   apr_app_initialize(argc, argv, NULL);
   atexit(apr_terminate);

   apr_pool_create(pool, NULL);

   for(i = 0; i  100; i++) {
   apr_thread_t *thread = NULL;
   apr_thread_create(thread, NULL, thread_worker, NULL, pool);
   }

   apr_sleep(1000 * 1000 * 1000);
}



I believe that's the stack reserved for each thread.
You can verify that by using either pmap or using ulimit to adjust the 
value and you will see the difference.


HTH,
Henry

thread pool patch again

[Henry Jen]

Hi,

Another attempt with checking result of palloc/pcalloc(thanks Davi 
pointing those out).


Cheers,
Henry
--- aprutil.dsp	(revision 408760)
+++ aprutil.dsp	(working copy)
@@ -240,6 +240,10 @@
 # PROP Default_Filter 
 # Begin Source File
 
+SOURCE=.\misc\apr_thread_pool.c
+# End Source File
+# Begin Source File
+
 SOURCE=.\misc\apr_date.c
 # End Source File
 # Begin Source File
@@ -512,6 +516,10 @@
 # End Source File
 # Begin Source File
 
+SOURCE=.\include\apr_thread_pool.h
+# End Source File
+# Begin Source File
+
 SOURCE=.\include\apr_date.h
 # End Source File
 # Begin Source File
--- /dev/null	2006-05-22 13:58:50.0 -0700
+++ include/apr_thread_pool.h	2006-05-22 13:44:26.963217000 -0700
@@ -0,0 +1,199 @@
+/* Copyright 2006 Sun Microsystems, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License);
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy and the
+ * number of tasks in the queue is higher than the adjustable threshold, the
+ * pool will try to create a new thread to serve the task if the maximum number
+ * has not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C
+{
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct apr_thread_pool apr_thread_pool_t;
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Create a thread pool
+ * @param me A pointer points to the pointer recieves the created
+ * apr_thread_pool object. The returned value will be NULL if failed to create
+ * the thread pool.
+ * @param init_threads The number of threads to be created initially, the number
+ * will also be used as the initial value for maximum number of idle threads. 
+ * @param max_threads The maximum number of threads that can be created
+ * @param pool The pool to use
+ * @return APR_SUCCESS if the thread pool was created successfully. Otherwise,
+ * the error code.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
+ apr_size_t init_threads,
+ apr_size_t max_threads,
+ apr_pool_t * pool);
+
+/**
+ * Destroy the thread pool and stop all the threads
+ * @return APR_SUCCESS if all threads are stopped.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t * me);
+
+/**
+ * Schedule a task to the bottom of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @return APR_SUCCESS if the task had been scheduled successfully
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t * me,
+   apr_thread_start_t func,
+   void *param,
+   apr_byte_t priority);
+
+/**
+ * Schedule a task to the top of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @return APR_SUCCESS if the task had been scheduled successfully
+ */
+APR_DECLARE(apr_status_t) 

Thread pool test

[Henry Jen]

Hi,

Attached is a test program for the thread pool, it's really simple and 
informal, but does at least can be used to verify if tasks are performed 
in order and threads are working as expected.


As I said, it's a quick hack and not really a formal functional test. So 
feel free to improve it. :-)


Cheers,
Henry

Checking expat on Solaris

[Henry Jen]

Hi,

The attached patch file was reviewed, my last reply lead to no further 
discussion.


So here it is again, I removed the controversial -R flag in the result 
LDFLAGS for now, but wondering what should I do to convince you the -R 
is necessary for Solaris and cause no harm on others?


As I explained and quoted from the man page, -R is not needed if you are 
link against a libtool .la file, but in case you are link against a .so 
file, it is needed.


Cheers,
Henry
Index: build/apu-conf.m4
===
--- build/apu-conf.m4	(revision 408760)
+++ build/apu-conf.m4	(working copy)
@@ -63,27 +63,24 @@
 expat_ldflags=-L$1/lib
 expat_libs=-lexpat
 expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.la; then
-dnl Expat 1.95.* installation (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib64/libexpat.la; then
-dnl Expat 1.95.* installation on certain 64-bit platforms (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib64
-expat_libs=-lexpat
-expat_libtool=$1/lib64/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.a; then
-dnl Expat 1.95.* installation (without libtool)
-dnl FreeBSD textproc/expat2
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
+  elif test -r $1/include/expat.h; then
+old_ldflags=$LDFLAGS
+old_cflags=$CFLAGS
+for d in $1/lib $1/libdir ; do
+  CFLAGS=$old_cflags -I$1/include
+  LDFLAGS=$old_ldflags -L$d -R$d
+  AC_CHECK_LIB(expat, XML_ParserCreate, [
+expat_include_dir=$1/include
+expat_libs=-lexpat
+expat_ldflags=-L$d
+if test -r $d/libexpat.la; then
+  expat_libtool=$d/libexpat.la
+fi
+break
+  ], [])
+done
+CFLAGS=$old_cflags
+LDFLAGS=$old_ldflags
   elif test -r $1/xmlparse.h; then
 dnl maybe an expat-lite. use this dir for both includes and libs
 expat_include_dir=$1

[Fwd: Re: Ack: CLA for Henry Jen [was Re: thread pool status]]

[Henry Jen]

William A. Rowe, Jr. wrote:

+notinavail:Henry Jen:Henry Jen:[EMAIL PROTECTED]:Signed CLA

Is received - for the benefit of anyone with cycles to commit his thread
pool design or any other code contributions.



Attached is the latest patch for the thread pool implementation, your 
help on review/commit it is greatly appreciated.


Cheers,
Henry

--- aprutil.dsp	(revision 408760)
+++ aprutil.dsp	(working copy)
@@ -240,6 +240,10 @@
 # PROP Default_Filter 
 # Begin Source File
 
+SOURCE=.\misc\apr_thread_pool.c
+# End Source File
+# Begin Source File
+
 SOURCE=.\misc\apr_date.c
 # End Source File
 # Begin Source File
@@ -512,6 +516,10 @@
 # End Source File
 # Begin Source File
 
+SOURCE=.\include\apr_thread_pool.h
+# End Source File
+# Begin Source File
+
 SOURCE=.\include\apr_date.h
 # End Source File
 # Begin Source File
--- /dev/null	2006-05-22 13:58:40.0 -0700
+++ misc/apr_thread_pool.c	2006-05-22 13:44:20.219558000 -0700
@@ -0,0 +1,565 @@
+/* Copyright 2006 Sun Microsystems, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License);
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include assert.h
+#include apr_thread_pool.h
+#include apr_ring.h
+#include apr_thread_cond.h
+
+#if APR_HAS_THREADS
+
+#define TASK_PRIORITY_SEGS 4
+#define TASK_PRIORITY_SEG(x) (((x)-priority  0xFF) / 64)
+
+typedef struct apr_thread_pool_task
+{
+APR_RING_ENTRY(apr_thread_pool_task) link;
+apr_thread_start_t func;
+void *param;
+apr_byte_t priority;
+} apr_thread_pool_task_t;
+
+APR_RING_HEAD(apr_thread_pool_tasks, apr_thread_pool_task);
+
+struct apr_thread_list_elt
+{
+APR_RING_ENTRY(apr_thread_list_elt) link;
+apr_thread_t *thd;
+volatile int stop;
+};
+
+APR_RING_HEAD(apr_thread_list, apr_thread_list_elt);
+
+struct apr_thread_pool
+{
+apr_pool_t *pool;
+volatile apr_size_t thd_max;
+volatile apr_size_t idle_max;
+volatile apr_size_t thd_cnt;
+volatile apr_size_t idle_cnt;
+volatile apr_size_t task_cnt;
+volatile apr_size_t threshold;
+struct apr_thread_pool_tasks *tasks;
+struct apr_thread_list *busy_thds;
+struct apr_thread_list *idle_thds;
+apr_thread_mutex_t *lock;
+apr_thread_mutex_t *cond_lock;
+apr_thread_cond_t *cond;
+volatile int terminated;
+struct apr_thread_pool_tasks *recycled_tasks;
+apr_thread_pool_task_t *task_idx[TASK_PRIORITY_SEGS];
+};
+
+static apr_status_t thread_pool_construct(apr_thread_pool_t * me,
+  apr_size_t init_threads,
+  apr_size_t max_threads)
+{
+apr_status_t rv;
+int i;
+
+me-thd_max = max_threads;
+me-idle_max = init_threads;
+me-threshold = init_threads / 2;
+rv = apr_thread_mutex_create(me-lock, APR_THREAD_MUTEX_NESTED,
+ me-pool);
+if (APR_SUCCESS != rv) {
+return rv;
+}
+rv = apr_thread_mutex_create(me-cond_lock, APR_THREAD_MUTEX_UNNESTED,
+ me-pool);
+if (APR_SUCCESS != rv) {
+apr_thread_mutex_destroy(me-lock);
+return rv;
+}
+rv = apr_thread_cond_create(me-cond, me-pool);
+if (APR_SUCCESS != rv) {
+apr_thread_mutex_destroy(me-lock);
+apr_thread_mutex_destroy(me-cond_lock);
+return rv;
+}
+me-tasks = apr_palloc(me-pool, sizeof(*me-tasks));
+APR_RING_INIT(me-tasks, apr_thread_pool_task, link);
+me-recycled_tasks = apr_palloc(me-pool, sizeof(*me-recycled_tasks));
+APR_RING_INIT(me-recycled_tasks, apr_thread_pool_task, link);
+me-busy_thds = apr_palloc(me-pool, sizeof(*me-busy_thds));
+APR_RING_INIT(me-busy_thds, apr_thread_list_elt, link);
+me-idle_thds = apr_palloc(me-pool, sizeof(*me-idle_thds));
+APR_RING_INIT(me-idle_thds, apr_thread_list_elt, link);
+me-thd_cnt = me-idle_cnt = me-task_cnt = 0;
+me-terminated = 0;
+for (i = 0; i  TASK_PRIORITY_SEGS; i++) {
+me-task_idx[i] = NULL;
+}
+return APR_SUCCESS;
+}
+
+/*
+ * NOTE: This function is not thread safe by itself. Caller should hold the lock
+ */
+static apr_thread_pool_task_t *pop_task(apr_thread_pool_t * me)
+{
+apr_thread_pool_task_t *task;
+int seg;
+
+if (0 == me-task_cnt) {
+return NULL;
+}
+
+task = APR_RING_FIRST(me-tasks);
+--me-task_cnt;
+seg = TASK_PRIORITY_SEG(task);
+if (task == me-task_idx[seg]) {
+me-task_idx[seg] = APR_RING_NEXT(task, link);
+if (me-task_idx[seg] == 

Re: [Fwd: Re: Ack: CLA for Henry Jen [was Re: thread pool status]]

[Henry Jen]

Davi Arnaut wrote:

On Mon, 22 May 2006 15:07:56 -0700
Henry Jen [EMAIL PROTECTED] wrote:


William A. Rowe, Jr. wrote:

+notinavail:Henry Jen:Henry Jen:[EMAIL PROTECTED]:Signed CLA

Is received - for the benefit of anyone with cycles to commit his thread
pool design or any other code contributions.

Attached is the latest patch for the thread pool implementation, your 
help on review/commit it is greatly appreciated.


Cheers,
Henry




Glimpse:

+static apr_status_t thread_pool_cleanup(void *me)
+{
+apr_thread_pool_t *_self = me;
+
+_self-terminated = 1;
+apr_thread_pool_idle_max_set(_self, 0);
+while (_self-thd_cnt) {
+apr_sleep(20 * 1000);   /* spin lock with 20 ms */
+}

What happens to the busy threads ?


Those will stop after done current task at hand with the terminated 
flag. Therefore a spin lock. :-)




+
+*me = apr_pcalloc(pool, sizeof(**me));
+if (!*me) {
+return APR_ENOMEM;
+}

Be concise, either check all apr_pcalloc failures or none.



I intent to check, I spot one place I did not in thread_pool_func, 
anywhere else you noticed?


It was brought up earlier that apache has a convention not to check, but 
some application needs to be able to stop gracefully even with error 
condition happens. So, is checking acceptable to apr?




+
+while (init_threads) {
+++(*me)-thd_cnt;
+rv = apr_thread_create(t, NULL, thread_pool_func, *me, (*me)-pool);
+if (APR_SUCCESS == rv) {
+--init_threads;
+}
+}

What if apr_thread_create fails ? thd_cnt will hold a wrong value. Also, a small
apr_sleep() here wouldn't hurt.

+if (0  next) {

Excuse me, but this is just too much :)



Okay, okay. I will change it to 'if (next  0)'. :-)



+++me-thd_cnt;
+apr_thread_create(thd, NULL, thread_pool_func, me, me-pool);
+}

apr_thread_create may fail..



Thanks, I will fix it.

Cheers,
Henry

Re: Google Summer of Code Applications Submitted for apr-build-system and apr-logging

[Henry Jen]

Curt Arnold wrote:

On the logging proposal, I see a decent amount of discussion regarding 
the Windows Event Log methods, but little regarding OutputDebugString or 
TraceEvent which a native Windows app would use to output debug level 
message or lower.  The Event Log methods are way too expensive for 
diagnostic logging.  I'd assume a typical Windows configuration would do 
something send info and higher messages to the Event Log and debug and 
lower severity messages to OutputDebugString or TraceEvent.




Good idea.

Cheers,
Henry

Re: Google Summer of Code Applications Submitted for apr-build-system and apr-logging

[Henry Jen]

Walter Mundt wrote:

Henry Jen wrote:
AFAIK, Windows event logging API supports Event Source and Unix syslog 
supports facility and a ident string. So I believe this should be 
implemented in a Platform-dependent way instead of being part of the 
message for a platform independent approach.


True.  Some of this is added in my proposal on the wiki.  After some 
further looking about on MSDN, however, I've noticed that the Windows 
event sources must be individually predefined in the registry.  As such, 
I've stuck with a set of them that correspond to the Unix syslog 
facility constants for the API.  I may add a prefix option to the 
constructors to correspond to the Unix ident string.




As long as it allows multiple log to be opened and each can have it's 
own ident string, I am happy with that. :-)


Sorry for pitching the jxta logging API again. The selector API is for 
this purpose so that you can choose what to be logged based on both 
facility and level. Which is basically same thing to the selector in 
syslog.conf man page.


Given the extremely limited number of facilities, and the fact that I 
can't really see more than maybe two being used in a single application, 
the value of being able to filter log messages by facility within an 
application seems minimal to me.  I will happily listen to any arguments 
to the contrary, however.  For the moment, I've left it out of the 
design for simplicity's sake.




I guess this depends on which level the apr_log is at.

An application may have several components, and may need to use several 
facilities(the concept of facility here is more like the ident part, 
that's how I define the jxta API) to distinguish those components. 
Should they have different logs or a log support multiple facilities?


If I add log in the apr/apr-util to aid debugging, I may have facility 
like thread/queue/ldap etc. But you can argue that this is not what log 
is for. :-P


One facility per log works, just a little bit more to be done in the 
application. For example, when you want to filter the log, you got 
multiple logs to do.


Anyway, that maybe beyond apr's scope for logging. But with those two 
features serve really well in the two projects I have been working on. 
Would be shame if I need to add another portable logging layer for this 
minimum feature set. :-)



Another thing is that, it would be much convenient to allow 
apr_snprintf style logging. What I am saying is


apr_status_t apr_log_append(const char *cat, int level, const char 
*fmt, ...);


Done!  I can't believe I overlooked this.  In fact, such a function is 
so useful that I've decided to see if I can get away with just calling 
it apr_log(). ;)



Another API might be useful is to close an log file early. That is,

apr_status_t apr_log_close(apr_log_t *log);


Oops, definitely.  It's not on there, but I'll put it in with the next 
round of changes; I'm assuming that you'll have a few more points to 
bring up shortly.




Not really. :-)

Cheers,
Henry

Re: Google Summer of Code Applications Submitted for apr-build-system and apr-logging

[Henry Jen]

Walter Mundt wrote:

Henry Jen wrote:
As long as it allows multiple log to be opened and each can have it's 
own ident string, I am happy with that. :-)


Okay, I'll add something like this sometime soon unless someone objects.



I guess this depends on which level the apr_log is at.

An application may have several components, and may need to use 
several facilities(the concept of facility here is more like the 
ident part, that's how I define the jxta API) to distinguish those 
components. Should they have different logs or a log support multiple 
facilities?


If I add log in the apr/apr-util to aid debugging, I may have facility 
like thread/queue/ldap etc. But you can argue that this is not what 
log is for. :-P


One facility per log works, just a little bit more to be done in the 
application. For example, when you want to filter the log, you got 
multiple logs to do.


Anyway, that maybe beyond apr's scope for logging. But with those two 
features serve really well in the two projects I have been working on. 
Would be shame if I need to add another portable logging layer for 
this minimum feature set. :-)


I'm not sure really what you mean like this.  Please look at the changed 
app on the wiki -- when *I* say facility what I mean is strictly Unix 
syslog facility; you seem to mean something different.  So there is a 
facility mail, user, daemon, etc...and _that is all_.  See your 
favorite 'man 3 syslog'.




Understand, and you are right, I mean different by facility. So maybe I 
should not use the term facility to avoid confusion.


The sole purpose is to identify the source, so the ident string is 
what really matters.


Beyond that, it seems to me that we end up right back in optimal 
cross-platform impl. territory.  The exception is that Windows event 
sources can be almost-arbitrary strings (they must be regex keys, so no 
\ or special chars.)...but see my previous comments on that.


Once again, I am absolutely fine with expanding the scope of the project 
to make apr-logging a generally useful logging API.  Right now, I see it 
more as a portability shim on top of which a full-fledged logging system 
might be built outside of APR.




The way I see it is not much different from yours. I see it as a 
portable layer for full-fledge logging systems, sort of like apr_dbd for 
different databases.


apr-log define a minimum set of logging functionality, the underlying 
log system can do things like rotate log files, maintaining extra 
logging contexts, etc.


One more consideration: I may add a single win32 specific 
'apr_log_win32_set_source' call; applications that want to use 
apr-logging AND have their own source field in the Windows event 
logger could add some conditionally-compiled calls to that function. 
That would then disable the use of the syslog-emulation sources for that 
log instance.


I wish we can somehow use ident string to register an event source on 
the fly in case that is not too much effort. Otherwise, a fixed set of 
source and embedded the ident string into the log message is good 
enough. Anyway, this is implementation detail to Windows. :-)


The progress so far looks really nice, good work.

Cheers,
Henry

Re: [PATCH] apr_network_io.h to include arpa/inet.h

[Henry Jen]

Colm MacCarthaigh wrote:

On Mon, May 08, 2006 at 09:15:26PM -0700, Henry Jen wrote:
On Solaris 11, the inet_addr is defined in arpa/inet.h. The attached 
patch include the needed header file.i


I've been compiling apr trunk on Solaris 11 for a few months now,
without this ever happening :/ Are you seeing this on a Solaris
11 beta? or a Nevada build? and which exact version, just so I
can test it :)

Oh, and you forgot the attachment!



Nevada b35.

Cheers,
Henry

Re: Google Summer of Code Applications Submitted for apr-build-system and apr-logging

[Henry Jen]

Walter Mundt wrote:
Thanks for the feedback on these applications.  I've refined them and 
submitted both via Google now.  Feel free to check the wiki page at 
http://wiki.apache.org/general/WalterMundt/SummerOfCodeProposals again 
for the update apps and please continue to provide any 
feedback/questions/ideas for improvement.


If you're signed up as an Apache mentor with Google, please also comment 
through their web application, as this allows me to submit updated 
versions of my applications for re-scoring, apparently even after the 
deadline for new applications.




I like the set_format part. :-)

A couple comments:

1. How do you distinguish the event source? Even in a single 
application, multiple categories may be desired to classify the events. 
Not to mention different apps.


2. This may only be me, but I would like to have capability to turn off 
some logging capability at runtime. It's kind of different verbose 
levels. With that, you can have more verbose logs to help you debugging 
an application when needed while a minimum when things are running fine.


Cheers,
Henry

Re: Google Summer of Code Applications Submitted for apr-build-system and apr-logging

[Henry Jen]

Walter Mundt wrote:

Henry Jen wrote:

A couple comments:

1. How do you distinguish the event source? Even in a single 
application, multiple categories may be desired to classify the 
events. Not to mention different apps.


There is no facility to do this currently.  I deliberately decided to 
leave that out for several reasons.  The most relevant for discussion on 
this list is that I tried to make the API so that it provides the 
minimal subset of logging functionality that must be implemented in a 
platform-dependent way.  Since context-aware logging functions could 
easily be built on top of this API without sacrificing any 
functionality, I left them out.




AFAIK, Windows event logging API supports Event Source and Unix syslog 
supports facility and a ident string. So I believe this should be 
implemented in a Platform-dependent way instead of being part of the 
message for a platform independent approach.


If there is a consensus among APR developers that more functionality 
(like this, but really anything useful that fits in the has an optimal 
platform-independent implementation mold) should be included, I have no 
problem adding it.




These 2 things are really all I needed for a basic logging API.

2. This may only be me, but I would like to have capability to turn 
off some logging capability at runtime. It's kind of different verbose 
levels. With that, you can have more verbose logs to help you 
debugging an application when needed while a minimum when things are 
running fine.


That's a very common usage scenario, and one that I allow via the level 
parameters on the log constructors.  However, I neglected to add a 
function for changing the logging level post-construction-time.  I'll 
add a spec for one on the wiki and in any revised app if someone 
comments through Google.




Sorry for pitching the jxta logging API again. The selector API is for 
this purpose so that you can choose what to be logged based on both 
facility and level. Which is basically same thing to the selector in 
syslog.conf man page.


So add an apr_log_set_selector(apr_log_t *log, const char* str) to your 
proposal would do it. :-)


Another thing is that, it would be much convenient to allow apr_snprintf 
style logging. What I am saying is


apr_status_t apr_log_append(const char *cat, int level, const char *fmt, 
...);


Another API might be useful is to close an log file early. That is,

apr_status_t apr_log_close(apr_log_t *log);

Cheers,
Henry

[PATCH] apr_network_io.h to include arpa/inet.h

[Henry Jen]

Hi,

On Solaris 11, the inet_addr is defined in arpa/inet.h. The attached 
patch include the needed header file.


Cheers,
Henry

Re: Patch for checking expat on Solaris

[Henry Jen]

Joe Orton wrote:

On Tue, May 02, 2006 at 01:10:44PM -0700, Henry Jen wrote:

Joe Orton wrote:

On Fri, Apr 28, 2006 at 07:06:07PM -0700, Henry Jen wrote:

Attached please find a new patch using AC_CHECK_LIB to find the correct
library path.
If you want to rewrite this code then look at dbm.m4 for how to do it 
properly.


- you can't just call AC_CHECK_LIB, you need to cache and display 
results properly
Uh? The result are cached in expat_xxx as they were before. Am I missing 
something?


Doing:

+for d in $1/lib $1/libdir ; do
+  CFLAGS=$old_cflags -I$1/include
+  LDFLAGS=$old_ldflags -L$d -R$d
+  AC_CHECK_LIB(expat, XML_ParserCreate, [

does not work as you might think.  AC_CHECK_LIB will cache the test 
result the first time through the for loop; the second test will always 
have the same result as the first.




Hmm, it won't because there is a break for the first match. Otherwise, 
no result is set.


- the -R flag should really never be used, especially like this - at the 
very least, you can't assume all linkers support it.  You generally can 
rely on libtool to add RPATHs iff necessary.
Hmm, I don't know about how libtool add RPATH. Does it apply to 
situation like this(solve dependencies on some other share libraries 
without a .la) or it only works for *creating* a .la?


libtool will add an RPATH to libaprutil if it is linked against a 
libexpat.la where the libexpat.la is in a directory which is not in the 
standard system library search path.




OK, what if it is not link against with libexpat.la but a libexpat.so? 
My experiment on Solaris does not add -R automatically. (Solaris only 
provides libexpat.so, no libexpat.la)


Cheers,
Henry

[PATCH] thread pool implementation

[Henry Jen]

Hi,

Attached is the latest patch passed my sanity test. Previous version is 
not working. :-P


I also add an adjustable threshold for tasks in queue to control when 
new thread should be created. Default to half of the idle 
threads(initial number of threads).


Cheers,
Henry
--- /dev/null	2006-05-03 09:36:00.0 -0700
+++ include/apr_thread_pool.h	2006-05-03 09:28:55.095403000 -0700
@@ -0,0 +1,199 @@
+/* Copyright 2006 Sun Microsystems, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License);
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy and the
+ * number of tasks in the queue is higher than the adjustable threshold, the
+ * pool will try to create a new thread to serve the task if the maximum number
+ * has not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C
+{
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct _apr_thread_pool apr_thread_pool_t;
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Create a thread pool
+ * @param me A pointer points to the pointer recieves the created
+ * apr_thread_pool object. The returned value will be NULL if failed to create
+ * the thread pool.
+ * @param pool The pool to use
+ * @param init_threads The number of threads to be created initially, the number
+ * will also be used as the initial value for maximum number of idle threads. 
+ * @param max_threads The maximum number of threads that can be created
+ * @return APR_SUCCESS if the thread pool was created successfully. Otherwise,
+ * the error code.
+ */
+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);
+
+/**
+ * Destroy the thread pool and stop all the threads
+ * @return APR_SUCCESS if all threads are stopped.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t * me);
+
+/**
+ * Schedule a task to the bottom of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @return APR_SUCCESS if the task had been scheduled successfully
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t * me,
+   apr_thread_start_t func,
+   void *param,
+   apr_byte_t priority);
+
+/**
+ * Schedule a task to the top of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @return APR_SUCCESS if the task had been scheduled successfully
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_top(apr_thread_pool_t * me,
+  apr_thread_start_t func,
+  void *param,
+  apr_byte_t priority);
+
+/**
+ * Get current number of tasks waiting in the queue
+ * @param me The 

Re: Thread pool prototype

[Henry Jen]

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

Re: Google Summer Of Code APR Applications

[Henry Jen]

Walter Mundt wrote:
As some of you are aware, the ASF is participating in the Google Summer 
of Code, and two project ideas for work on APR are posted on the ASF 
wiki.  I'm planning on submitting Summer of Code applications for both 
(they look equally interesting to me), and I'd like some feedback before 
I submit them.


I've posted drafts of them on the aforementioned wiki at 
http://wiki.apache.org/general/WalterMundt/SummerOfCodeProposals for 
your perusal and flaming pleasure.  Anyone is welcome and encouraged to 
read them and send me suggestions, either via this list or directly to 
my address.  Please note anything you'd like me to add/improve/expand 
upon (aside from the logging API design, though suggestions on how to 
approach that are welcome too!).


I know the first proposal is pretty sketchy, but I'm not sure how much 
more there is to add; it's one of those things that's really easy to 
describe even though it will be difficult to implement.


Note that a short bio is available in the parent user topic, and will be 
attached to the submitted applications; feel free to look at/ask 
questions about that as well.





Huh? logging API is in SoC?

I submitted a patch to support logging in apr about an year ago, which 
is now used extensively in jxta-c project. I was told logging project 
maybe a more appropriate place. :-) Anyway, you can find the API in 
attached header file, you can also check out the implementation at

http://jxta-c.jxta.org/source/browse/jxta-c/src/jxta_log.c?rev=1.26view=markup

If people are interested, I can sent out the patch again, and the SoC 
applicant can work to improve that.


It already include file based logging, and can be expanded to support 
unix syslog and Windows event log.


Cheers,
Henry
/*
 * Copyright (c) 2004 Henry Jen. All rights reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the License);
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an AS IS BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *
 */

#ifndef __JXTA_LOG_H__
#define __JXTA_LOG_H__

#include stdarg.h

#include jxta_types.h
#include jxta_errno.h

#ifdef __cplusplus
extern C {
#if 0
};
#endif
#endif

enum Jxta_log_levels {
JXTA_LOG_LEVEL_INVALID = -1,
JXTA_LOG_LEVEL_MIN = 0,
JXTA_LOG_LEVEL_FATAL = 0,
JXTA_LOG_LEVEL_ERROR,
JXTA_LOG_LEVEL_WARNING,
JXTA_LOG_LEVEL_INFO,
JXTA_LOG_LEVEL_DEBUG,
JXTA_LOG_LEVEL_TRACE,
JXTA_LOG_LEVEL_PARANOID,
JXTA_LOG_LEVEL_MAX
};

typedef enum Jxta_log_levels Jxta_log_level;

#define JXTA_LOG_LEVEL_FLAG_FATAL 1
#define JXTA_LOG_LEVEL_FLAG_ERROR 2
#define JXTA_LOG_LEVEL_FLAG_WARNING 4
#define JXTA_LOG_LEVEL_FLAG_INFO 8
#define JXTA_LOG_LEVEL_FLAG_DEBUG 16
#define JXTA_LOG_LEVEL_FLAG_TRACE 32
#define JXTA_LOG_LEVEL_FLAG_PARANOID 64

/* Mask for all level less then specified */
#define JXTA_LOG_LEVEL_MASK_NONE 0
#define JXTA_LOG_LEVEL_MASK_FATAL 1
#define JXTA_LOG_LEVEL_MASK_ERROR 3
#define JXTA_LOG_LEVEL_MASK_WARNING 7
#define JXTA_LOG_LEVEL_MASK_INFO 15
#define JXTA_LOG_LEVEL_MASK_DEBUG 31
#define JXTA_LOG_LEVEL_MASK_TRACE 63
#define JXTA_LOG_LEVEL_MASK_PARANOID 127
#define JXTA_LOG_LEVEL_MASK_ALL 127

/**
 * Prototype of the logger function
 *
 * @param void * user data, typically would be a struct holding the
 *data needed by the logger function
 * @param const char * cat, a character string can be used to
 *categorize the log entriedi, also can be used by a selector to
 *decide whether the message should be logged.
 * @param int level, the level of the log message, can be used by a
 *selector to decide whether the message should be logged.
 * @param const char * fmt, the format string
 * @param va_list ap, the arguments for the format string
 *
 * @return Status code
 */
typedef Jxta_status(JXTA_STDCALL * Jxta_log_callback) (void *user_data, const char *cat, int level, const char *msg);

JXTA_DECLARE(Jxta_status) jxta_log_initialize(void);
JXTA_DECLARE(void) jxta_log_terminate(void);

/**
 * Register the logger function
 *
 * @param Jxta_log_callback, the logger function
 * @param void * user data, typically would be a struct holding the
 *data needed by the logger function
 */
JXTA_DECLARE(void) jxta_log_using(Jxta_log_callback log_cb, void *user_data);

/**
 * Call this function to append log with the global registered callback
 * logger function
 */
JXTA_DECLARE(Jxta_status) jxta_log_append(const char *cat, int level, const char *fmt, ...);

/**
 * Call this function to append log with the global registered callback
 * logger function
 */
JXTA_DECLARE(Jxta_status) jxta_log_appendv(const

Re: Google Summer Of Code APR Applications

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:


It already include file based logging, and can be expanded to support 
unix syslog and Windows event log.


Well, from an apr PoV, that's probably why the patch was ignored.  There
are constantly competeing forces at work to 1) make apr handle almost all
the dull tasks C programmers have to do, and 2) narrowly construe apr to
handle issues which need extra code to handle portability between 
platforms,

or bind diverse available library solutions into a unified interface.

So logging is outside the scope of apr in general, and a questionable fit
for apr-util;  HOWEVER, the unix syslog v.s. windows event log v.s. some
generic file-based logging for platforms which support neither, that's
a very appropriate fit into apr.  


I got confused. :-)

The logging code is about to provide an unified API for logging and 
allow the app to determine how actually to do logging. Very much what 
you said above.


For unix, provides a callback for syslog. On Windows, a callback for 
event log, and on others, use the file based callback.


Check out the code, it is not that much overhead.

Cheers,
Henry


Of course the code all exists already,
it's a matter of porting the code from httpd to apr (and then reducing
the complexity of httpd by using the new apr interfaces ;-)

Re: Thread pool prototype

[Henry Jen]

William A. Rowe, Jr. wrote:


Anyways, I ment to add, nice work thus far.  If you have yet to do so,
please submit your CLA to our secretary Jim so that your implementation
can be considered for commit.  (We are not picking about a few-line patch
here and there, but for major works, it's necessary.)  Find it here;
http://www.apache.org/licenses/#clas

I'm happy to commit this to a sandbox or trunk for now to let everyone
begin iteratively improving it, for adoption with a 1.3, 1.4 or 2.0 APR
release, once we think it's baked.  Just need the CLA first, and your
final draft (I'd hold off just a bit for feedback from more of the
peanut gallery.)



Attached please find the latest patch incorporate the feedbacks and bug 
fixes for adding a task.


I had fax in the CLA.

Cheers,
Henry
--- /dev/null	2006-05-02 10:28:27.0 -0700
+++ include/apr_thread_pool.h	2006-05-01 18:34:54.427538000 -0700
@@ -0,0 +1,151 @@
+/* Copyright 2006 Sun Microsystems, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License);
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy, the pool
+ * will try to create a new thread to serve the task if the maximum number has
+ * not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C
+{
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct _apr_thread_pool apr_thread_pool_t;
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Create a thread pool
+ * @param me A pointer points to the pointer recieves the created
+ * apr_thread_pool object. The returned value will be NULL if failed to create
+ * the thread pool.
+ * @param pool The pool to use
+ * @param init_threads The number of threads to be created initially, the number
+ * will also be used as the initial value for maximum number of idle threads. 
+ * @param max_threads The maximum number of threads that can be created
+ * @return APR_SUCCESS if the thread pool was created successfully. Otherwise,
+ * the error code.
+ */
+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);
+
+/**
+ * Destroy the thread pool and stop all the threads
+ * @return APR_SUCCESS if all threads are stopped.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t * me);
+
+/**
+ * Schedule a task to the bottom of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @return APR_SUCCESS if the task had been scheduled successfully
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t * me,
+   apr_thread_start_t func,
+   void *param,
+   apr_byte_t priority);
+
+/**
+ * Schedule a task to the top of the tasks of same priority.
+ * @param me The thread pool
+ * @param func The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of 

Re: Patch for checking expat on Solaris

[Henry Jen]

Joe Orton wrote:

On Fri, Apr 28, 2006 at 07:06:07PM -0700, Henry Jen wrote:

Attached please find a new patch using AC_CHECK_LIB to find the correct
library path.


If you want to rewrite this code then look at dbm.m4 for how to do it 
properly.


- you can't just call AC_CHECK_LIB, you need to cache and display 
results properly




Uh? The result are cached in expat_xxx as they were before. Am I missing 
something?


- the -R flag should really never be used, especially like this - at the 
very least, you can't assume all linkers support it.  You generally can 
rely on libtool to add RPATHs iff necessary.




Hmm, I don't know about how libtool add RPATH. Does it apply to 
situation like this(solve dependencies on some other share libraries 
without a .la) or it only works for *creating* a .la?


Cheers,
Henry

Thread pool prototype

[Henry Jen]

Hi,

Attached is a patch implement the proposed thread pool API, it is 
compilable and have *NOT* been tested. I post it here and hopefully get 
an early review.


Please let me know if you have any comments. :-)

Cheers,
Henry
--- apr-util-svn/include/apr_thread_pool.h  1969-12-31 16:00:00.0 
-0800
+++ apr-util/include/apr_thread_pool.h  2006-04-28 20:33:01.412857000 -0700
@@ -0,0 +1,168 @@
+/* Copyright 2006 Sun Microsystems, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License);
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef APR_THREAD_POOL_H
+#define APR_THREAD_POOL_H
+
+#include apr.h
+#include apr_thread_proc.h
+
+/**
+ * @file apr_thread_pool.h
+ * @brief APR Thread Pool Library
+
+ * @remarks This library implements a thread pool using apr_thread_t. A thread
+ * pool is a set of threads that can be created in advance or on demand until a
+ * maximum number. When a task is scheduled, the thread pool will find an idle
+ * thread to handle the task. In case all existing threads are busy, the pool
+ * will try to create a new thread to serve the task if the maximum number has
+ * not been reached. Otherwise, the task will be put into a queue based on
+ * priority, which can be valued from 0 to 255, with higher value been served
+ * first. In case there are tasks with the same priority, the new task is put 
at
+ * the top or the bottom depeneds on which function is used to put the task.
+ *
+ * @remarks There may be the case that a thread pool can use up the maximum
+ * number of threads at peak load, but having those threads idle afterwards. A
+ * maximum number of idle threads can be set so that extra idling threads will
+ * be terminated to save system resrouces. 
+ */
+#if APR_HAS_THREADS
+
+#ifdef __cplusplus
+extern C {
+#if 0
+};
+#endif
+#endif /* __cplusplus */
+
+/** Opaque Thread Pool structure. */
+typedef struct _apr_thread_pool apr_thread_pool_t;
+
+/**
+ * The prototype for any APR thread pool task functions.
+ * @param apr_thread_t the apr_thread_t structure for the thread is executing 
the task
+ * @param void * a pointer to the user data passed in when schedule the task
+ */
+typedef void* (APR_THREAD_FUNC *apr_thread_pool_task_t) (apr_thread_t *, void 
*);
+
+#define APR_THREAD_TASK_PRIORITY_LOWEST 0
+#define APR_THREAD_TASK_PRIORITY_LOW 63
+#define APR_THREAD_TASK_PRIORITY_NORMAL 127
+#define APR_THREAD_TASK_PRIORITY_HIGH 191
+#define APR_THREAD_TASK_PRIORITY_HIGHEST 255
+
+/**
+ * Setup all of the internal structures required to use thread pools
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_init(void);
+
+/**
+ * Tear down all of the internal structures required to use pools
+ */
+APR_DECLARE(void) apr_thread_pool_terminate(void);
+
+/**
+ * Create a thread pool
+ * @param pool The pool to use
+ * @param init_threads The number of threads to be created initially, the 
number
+ * will also be used as the initial value for maximum number of idle threads. 
+ * @param max_threads The maximum number of threads that can be created
+ * @param err Receive the error code, can be NULL if not needed.
+ * @return The thread pool. NULL if failed to create the thread pool. Put the
+ * error code in the err parameter if it is not NULL. 
+ */
+APR_DECLARE(apr_thread_pool_t*) apr_thread_pool_create(apr_pool_t *pool,
+   apr_size_t 
init_threads, 
+   apr_size_t max_threads, 
+   apr_status_t *err);
+
+/**
+ * Destroy the thread pool and stop all the threads
+ * @return APR_SUCCESS if all threads are stopped.
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t *me);
+
+/**
+ * Schedule a task to the bottom of the tasks of same priority.
+ * @param me The thread pool
+ * @param task The task function
+ * @param param The parameter for the task function
+ * @param priority The priority of the task.
+ * @return APR_SUCCESS if the task had been scheduled successfully
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t *me,
+   apr_thread_pool_task_t task, 
+   void *param, 
+   apr_byte_t priority);
+
+/**
+ * Schedule a task to the top of the tasks of same priority.
+ * @param me The thread pool
+ * @param task The task function
+ * @param param The parameter for the task function
+ * 

Re: Patch for checking expat on Solaris

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:

William A. Rowe, Jr. wrote:

The issue I have is that your -enforcing- -R destroys my 
-portability-.  We
need to find a happy comprimize to build to a manditory -prefix 
versus the

usual -prefix, yet relocatable (with LD_LIBRARY_PATH).


Hmm, I am curious what portability is destroyed. :-)




I misunderstood the portability you referring to, thought it means other
platforms. Glad to know that you are using Solaris and SunStudio. :-P

IIUC, before patch, my solaris binary can deployed at an arbitrary 
/opt/apache2/
location, apachectl resolves the LD_LIBRARY_PATH.  Before patch, your 
arbitrary
solution (linked to a thirdparty build of expat installed in 
/usr/local/lib or
some other, nonstandard, location) fails to resolve libexpat.so at run 
time.




-R gives a default search path, LIB_LIBRARY_PATH still can be used when
needed.

Quote `man ld.so.1` on Solaris:

 The runtime linker uses a prescribed search path for  locat-
 ing  the  dynamic  dependencies  of  an  object. The default
 search paths are the runpath recorded in  the  object,  fol-
 lowed  by  a  series  of  defaults.  For 32-bit objects, the
 defaults are /lib followed by /usr/lib. For 64-bit  objects,
 the  defaults  are  /lib/64  followed  by /usr/lib/64. These
 defaults component can be  modified  using  a  configuration
 file  that is created with crle(1). The runpath is specified
 when the dynamic object is constructed using the  -R  option
 to  ld(1).  The  environment variable LD_LIBRARY_PATH can be
 used to indicate  directories  to  be  searched  before  the
 default directories.


I work out the patch because I want to use the default expat installed
by Solaris system(in /usr/sfw/lib, which is not in the default search
path), not custom built or the one come with apr-util.

After patch, my solaris binary can no longer be redeployed to 
/opt/httpd/, but

your binary finds your expat in /opt/sfw/lib or where ever.



This should not be true, but I could be overlooking. Would you please
elaborate?

The difference between with or without -R is the -R will be searched 
before the system default. Unless you got a different copy in the -R 
pointed path, there should be no difference. But I wonder how could that 
happen, -R is about to make sure you are getting the correct copy.


So my point is, if you want to add -R explicitly to your ldflags, that's 
cool.

If you want to come up with a proposal to make this automatic but optional,
that's cool too.  If you unilaterally decide your way is right, that's 
not cool.




Your comment is very much appreciated, gives me a chance to exam my
understanding. :-)

Before we sort out the problem, how about let's remove the -R and commit
the rest? That at least allows configure to locate the lib correctly. I
will try to ping you on irc to have some discussion if you get some time.

As the coolness, I want to work with the community to make sure I am
doing the correct thing, which is why I always consult upstream team.
It's the advise I got from Solaris engineers to use -R instead of change
my system default setting with crle, and don't set LD_LIBRARY_PATH.
Following two URLs have covered the topic.

http://www.visi.com/~barr/ldpath.html
http://linuxmafia.com/faq/Admin/ld-lib-path.html

Thanks again for the comment, I really appreciate.

Cheers,
Henry

Re: Thread pool prototype

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:


Please let me know if you have any comments. :-)
+/**
+ * Setup all of the internal structures required to use thread pools
+ */
+APR_DECLARE(apr_status_t) apr_thread_pool_init(void);
+
+/**
+ * Tear down all of the internal structures required to use pools
+ */
+APR_DECLARE(void) apr_thread_pool_terminate(void);


Double initialization, double teardown issues?  Consider that my libfoo.so
may want to create a threadpool, I'm not privy to whether or not the 
application

that embedded libfoo.so actualy intialized the threadpool API itself.

Is this needed?  If not can we please chuck it?  I understand the desire to
anticipate the unanticipated, but this certainly will be a bigger 
problem if

it were used, than if we discover it's needed somehow.



Based on the implementation, it does not seem to be needed. I will take 
it out. So is the apr_thread_pool_task_t type definition. We can simply 
reuse the apr_thread_start_t.


As the multiple init/term, that would be carefully handled as 
apr_init/term if those function is actually in use. :-)



+APR_DECLARE(apr_thread_pool_t*) apr_thread_pool_create(apr_pool_t *pool,
+   apr_size_t 
init_threads, +   
apr_size_t max_threads, 
+   apr_status_t 
*err);


E!

Do you mean

APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t 
**threadpool,

 apr_pool_t *pool,
 apr_size_t init_threads,
 apr_size_t max_threads);

Please follow style guidelines, and precident, in your proposals :)



Is there a guideline other than 
http://httpd.apache.org/dev/styleguide.html?


I understand the precedent, but I thought there is no requirement. :-D
apr_hash_t *apr_hash_make (apr_pool_t *pool)

I prefer this way because it is clear that an app can check against the 
returned pointer to be NULL without worrying the value to be undefined 
when an error occurs.


That said, I will fix it.


+/**
+ * Get current number of tasks waiting in the queue
+ * @param me The thread pool
+ * @return Number of tasks in the queue
+ */
+APR_DECLARE(int) apr_thread_pool_tasks_cnt_get(apr_thread_pool_t *me);


Worst of all worlds.  It's not a property (e.g. get/set), it's a verb 
(please
*count* for me your entities), so apr_thread_pool_tasks_count( ) seems 
right.



+/**
+ * Get current number of idling thread
+ * @param me The thread pool
+ * @return Number of idling threads
+ */
+APR_DECLARE(int) apr_thread_pool_unused_cnt_get(apr_thread_pool_t *me);


ditto... _count() not cnt_get()



Thank you! Pardon me as not a native English speaker.


+/**
+ * Stop all unused threads. Ignore the maximum number of idling threads.
+ * @param me The thread pool
+ * @return The total number of threads stopped.
+ */
+APR_DECLARE(int) 
apr_thread_pool_stop_unused_threads(apr_thread_pool_t *me);


What's the difference between setting apr_thread_pool_unused_max_set() 
to 0,

and this function?  Can we drop it?

It seems this function would stop idling threads at this point in time, 
while

it makes much more design sense to set unused_max to 0, ensuring the unused
threads continue to be pruned as they finish their unit of work.

Thoughts?



I tend to agree with you. Just use the set_unused_max makes sense.

In a way they work the same. But there are a few interesting differences:

1. Set the unused max does not force the thread to stop, an unused 
thread kills itself when it becomes idle and realize there are enough 
idle threads. Thus no joining or any locking issue for using this function.


2. stop_unused_threads kills *all* unused threads regardless the unused_max.

Is that mechanism make sense or just adding complexity? Not sure. It is 
for occasions you want to kill all unused threads and allow them to grow 
later. But why does an app do that? Under a resource constraint 
environment, this can serve as a mechanism for error recovery. Of 
course, you can do that with two consequent call to set_unused_max.


Think of that, I forgot to signal the idle threads when new tasks are 
added! :-P


Cheers,
Henry

Re: Thread pool prototype

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:



In a way they work the same. But there are a few interesting differences:

1. Set the unused max does not force the thread to stop, an unused 
thread kills itself when it becomes idle and realize there are enough 
idle threads. Thus no joining or any locking issue for using this 
function.


Is it reasonable to do a quick reap when max() is adjusted?  E.g. if the
freecount  max then call reap?



Reasonable. Might not be optimal, but makes more sense. I feel it's kind 
of like async and sync debate.


2. stop_unused_threads kills *all* unused threads regardless the 
unused_max.


That's interesting - what purpose would this 'feature' serve?

Is that mechanism make sense or just adding complexity? Not sure. It 
is for occasions you want to kill all unused threads and allow them to 
grow later. But why does an app do that? Under a resource constraint 
environment, this can serve as a mechanism for error recovery. Of 
course, you can do that with two consequent call to set_unused_max.


Once you've hit the wall, bits like this stand no chance of fixing things.
Apache HTTP and APR have longstanding policies of simply bailing once
allocations have started to fail altogether.



This sounds familiar to me. I was debating whether an app should abort 
when failed to allocate memory. I want to abort the app but others think 
it is more desirable to allow the app deal with it and trying to keep 
the app alive. :-)


Admitted it is not guaranteed, but some times it may keep the system 
alive. Assume the app is not using thread pool to manage all threads, 
and imagine there are 15 unused threads, and the app needs a turn around 
buffer of 3. By stopping all of them may last the system a little bit 
longer.


It's like you moving furnitures around to make a space in a packed room. :-)

Anyway, this is a corner case and it can still be worked around with two 
consequent calls if we stop threads when adjust the unused_max. I also 
think typically an app will set the number at beginning and forget about it.


So, let's get rid of this function and depends on unused_max_set to 
stopping idle threads.



Think of that, I forgot to signal the idle threads when new tasks are 
added! :-P


Whoops!  :)

What would you think in general about changing 'unused' threads to 'free'
threads throughout the comments/implementation?



I was thinking either idle or free? Is free the general convention in apr?

Cheers,
Henry

Re: Patch for checking expat on Solaris

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:

  -L and -R should be redundant, no?

No, they are not. -L is for link time, -R for runtime. For the 
default

linker on Solaris, the -R is taking the value from -L as gnu ld.

I meant to say Solaris default linker does not behave the same as GNU ld.
-R is recommended practice on Solaris. crle or LD_LIBRARY_PATH can do 
the trick, but those practice are discouraged.


The issue I have is that your -enforcing- -R destroys my -portability-.  We
need to find a happy comprimize to build to a manditory -prefix versus the
usual -prefix, yet relocatable (with LD_LIBRARY_PATH).





Hmm, I am curious what portability is destroyed. :-)

I mean, based on my limited platform experience, gcc supports -R and 
Windows is not using autotools, right? And if you built with the -L 
path, isn't it should be the same for -R?


I can work around this by using LDFLAGS but that may not be as precise 
as this.


Any other suggestions?

Cheers,
Henry

Re: Makefile.in does not take external LIBS LDFLAGS

[Henry Jen]

Ryan Bloom wrote:
Didn't we solve this problem by adding EXTRA_LIBS and EXTRA_LDFLAGS? I 
haven't looked at the code, but I remember (from years past), that

we didn't want people to set CFLAGS, LIBS, LDFLAGS, etc for some
reason.  Instead, we asked people to set EXTRA_CFLAGS, etc and we
picked those up.



I am relative new to apr, so not sure about history and no one pointed 
me to EXTRA_xxx. :-)


Include CFLAGS may not be appropriate, but I think LDFLAGS/LIBS should 
be OK although I don't feel very comfortable with it. Would some one 
please enlighten me a bit on this issue?


Have a quick look at those EXTRA flags, don't know how to use those. Ideas?

Cheers,
Henry



I could be mis-remembering though.

Ryan

On 4/28/06, Henry Jen [EMAIL PROTECTED] wrote:

Here is the new patch.

Cheers,
Henry



On 4/9/06, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
 Henry Jen wrote:
 
  will not have the -R flag in the Makefile.

 Your patch is invalid because we need to proxy the whole accumulated
 LDFLAGS off to the client who's trying to compile against apr[-util].
 Fixing Makefile isn't enough, LDFLAGS must be 'sticky' within the
 accumulated apr-1-config/apu-1-config syntax.

 Please revisit your patch and determine where your desired flags are
 being dumped by autoconf, and I'd be happy to commit a patch that
 ensures those user-given flags percolate into our APRUTIL_LDFLAGS
 throughout the configuration, as opposed to committing a bandaid.

 Yours,

 Bill







--
Ryan Bloom
[EMAIL PROTECTED]
[EMAIL PROTECTED]
[EMAIL PROTECTED]

Re: [PATCH] apr_thread_yield with pthread

[Henry Jen]

William A. Rowe, Jr. wrote:

This has been sitting out a while.  Any comments from the vocal majority
on their favorite platform?  Seems sane to me, but needs eyes from folks
intimately familiar with the pthreads implementation.

I noticed more are trying to purge the bug queue so thought it's a good
time to mention again.



+1 on Solaris 11.

Cheers,
Henry


Bill

 Original Message 
Subject: [PATCH] apr_thread_yield with pthread
Date: Mon, 27 Feb 2006 17:19:45 +0900
From: Keisuke Nishida [EMAIL PROTECTED]
To: dev@apr.apache.org

Hi,

I'm new to APR.  I've started using apr-1.2.2 as well as the trunk.

I found the definition of apr_thread_yield is empty in
apr/threadproc/unix/thread.c.
Is there any reason why not to call pthread_yield or sched_yield?

The attached patch does what I want.

Best regards,
Keisuke Nishida

Re: Makefile.in does not take external LIBS LDFLAGS

[Henry Jen]

Here is the new patch.Cheers,HenryOn 4/9/06, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
Henry Jen wrote: will not have the -R flag in the Makefile.Your patch is invalid because we need to proxy the whole accumulated
LDFLAGS off to the client who's trying to compile against apr[-util].Fixing Makefile isn't enough, LDFLAGS must be 'sticky' within theaccumulated apr-1-config/apu-1-config syntax.Please revisit your patch and determine where your desired flags are
being dumped by autoconf, and I'd be happy to commit a patch thatensures those user-given flags percolate into our APRUTIL_LDFLAGSthroughout the configuration, as opposed to committing a bandaid.Yours,
Bill
Index: configure.in
===
--- configure.in	(revision 398058)
+++ configure.in	(working copy)
@@ -97,6 +97,10 @@
 APR_ADDTO(CFLAGS, `$apr_config --cflags`)
 APR_ADDTO(CPPFLAGS, `$apr_config --cppflags`)
 
+dnl carry user defined LDFLAGS
+APR_ADDTO(APRUTIL_LDFLAGS, $LDFLAGS)
+APR_ADDTO(APRUTIL_LIBS, $LIBS)
+
 dnl
 dnl  Find the APR-ICONV directory.
 dnl

Re: Patch for checking expat on Solaris

[Henry Jen]

On 4/10/06, Henry Jen [EMAIL PROTECTED] wrote:
William A. Rowe, Jr. wrote: Henry Jen wrote: +elif test -r $1/include/expat.h; then +dnl Expat 1.95.* installation (without libtool) +dnl Solaris
 +expat_include_dir=$1/include +expat_ldflags=-L$1/lib -R$1/lib +expat_libs=-lexpat Here's my essential problems with growing this cruft further :(
 -L and -R should be redundant, no?No, they are not. -L is for link time, -R for runtime. For the defaultlinker on Solaris, the -R is taking the value from -L as gnu ld.
I meant to say Solaris default linker does not behave the same as GNU ld. -R is recommended practice on Solaris. crle or LD_LIBRARY_PATH can do the trick, but those practice are discouraged.
 Further, there's this lovely assumption that because we find an expat .h in X/include/ we infer without any testing whatsoever there is a ld in X/lib. These sorts of fragile assumptions *throughout* the build system (not
 singling out specifically your patch), e.g. the old uuid detection code, ongoing debates about lib64 (you assume $1/lib, how do you know X/include/ isn't X/lib64/'s?) Make me really hesitate to touch the code further without a real
 test-compile and test-link as the new uuid code performs (before bailing with 'I surrender!')Thank you for the feedback, I will redo the patch.IMHO, we should do AC_CHECK_LIB with the default LDFLAGS/CFLAGS appended
by the value of --with-expat suffixed with /include|/liband then makesure those flags are passed into the result Makefiles.Depending on the .a or .la file is in the place and readable is not morecorrect, I would think. Even if we want to do that, should we check for
.so rather than .la or .a? Because the .a or .la should not be necessaryif the libexpat.so is built correctly. Take the Solaris example, thereis no .la or .a, only .so exist in /usr/sfw/lib, and the link will succeed.
Attached please find a new patch using AC_CHECK_LIB to find the correct library path. Cheers,Henry
Index: build/apu-conf.m4
===
--- build/apu-conf.m4	(revision 398058)
+++ build/apu-conf.m4	(working copy)
@@ -63,27 +63,24 @@
 expat_ldflags=-L$1/lib
 expat_libs=-lexpat
 expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.la; then
-dnl Expat 1.95.* installation (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
-expat_libtool=$1/lib/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib64/libexpat.la; then
-dnl Expat 1.95.* installation on certain 64-bit platforms (with libtool)
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib64
-expat_libs=-lexpat
-expat_libtool=$1/lib64/libexpat.la
-  elif test -r $1/include/expat.h -a \
--r $1/lib/libexpat.a; then
-dnl Expat 1.95.* installation (without libtool)
-dnl FreeBSD textproc/expat2
-expat_include_dir=$1/include
-expat_ldflags=-L$1/lib
-expat_libs=-lexpat
+  elif test -r $1/include/expat.h; then
+old_ldflags=$LDFLAGS
+old_cflags=$CFLAGS
+for d in $1/lib $1/libdir ; do
+  CFLAGS=$old_cflags -I$1/include
+  LDFLAGS=$old_ldflags -L$d -R$d
+  AC_CHECK_LIB(expat, XML_ParserCreate, [
+expat_include_dir=$1/include
+expat_libs=-lexpat
+expat_ldflags=-L$d -R$d
+if test -r $d/libexpat.la; then
+  expat_libtool=$d/libexpat.la
+fi
+break
+  ], [])
+done
+CFLAGS=$old_cflags
+LDFLAGS=$old_ldflags
   elif test -r $1/xmlparse.h; then
 dnl maybe an expat-lite. use this dir for both includes and libs
 expat_include_dir=$1
Index: build/dbd.m4
===
--- build/dbd.m4	(revision 398058)
+++ build/dbd.m4	(working copy)
@@ -133,12 +133,12 @@
   apu_have_sqlite3=0
 else
   CPPFLAGS=-I$withval/include
-  LDFLAGS=-L$withval/lib 
+  LDFLAGS=-L$withval/lib -R$withval/lib
 
   AC_MSG_NOTICE(checking for sqlite3 in $withval)
   AC_CHECK_HEADER(sqlite3.h, AC_CHECK_LIB(sqlite3, sqlite3_open, [apu_have_sqlite3=1]))
   if test $apu_have_sqlite3 != 0; then
-APR_ADDTO(APRUTIL_LDFLAGS, [-L$withval/lib])
+APR_ADDTO(APRUTIL_LDFLAGS, [-L$withval/lib -R$withval/lib])
 APR_ADDTO(APRUTIL_INCLUDES, [-I$withval/include])
   fi
 fi

Re: Patch for checking expat on Solaris

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:


+  elif test -r $1/include/expat.h; then +dnl Expat 1.95.* 
installation (without libtool)

+dnl Solaris
+expat_include_dir=$1/include
+expat_ldflags=-L$1/lib -R$1/lib
+expat_libs=-lexpat


Here's my essential problems with growing this cruft further :(

-L and -R should be redundant, no?  


No, they are not. -L is for link time, -R for runtime. For the default
linker on Solaris, the -R is taking the value from -L as gnu ld.


Further, there's this lovely assumption
that because we find an expat .h in X/include/ we infer without any testing
whatsoever there is a ld in X/lib.

These sorts of fragile assumptions *throughout* the build system (not 
singling out specifically your patch), e.g. the old uuid detection code, 
ongoing debates
about lib64 (you assume $1/lib, how do you know X/include/ isn't 
X/lib64/'s?)
Make me really hesitate to touch the code further without a real 
test-compile
and test-link as the new uuid code performs (before bailing with 'I 
surrender!')




Thank you for the feedback, I will redo the patch.

IMHO, we should do AC_CHECK_LIB with the default LDFLAGS/CFLAGS appended 
by the value of --with-expat suffixed with /include|/liband then make 
sure those flags are passed into the result Makefiles.


Depending on the .a or .la file is in the place and readable is not more 
correct, I would think. Even if we want to do that, should we check for 
.so rather than .la or .a? Because the .a or .la should not be necessary 
if the libexpat.so is built correctly. Take the Solaris example, there 
is no .la or .a, only .so exist in /usr/sfw/lib, and the link will succeed.


Cheers,
Henry

Re: API Proposal for apr_thread_pool

[Henry Jen]

Henry Jen wrote:

William A. Rowe, Jr. wrote:

Henry Jen wrote:



/**
 * Stop all unused threads. Ignore the maximum number of idling threads.
 * @return The total number of threads stopped.
 */
APR_DECLARE(int) apr_thread_pool_stop_unused_threads(void);


I'm a little confused in your proposal between 
apr_thread_pool_terminate(),

apr_thread_pool_destroy(), and this function.

Can't this be reduced to



My mistake. The last couple functions should take a apr_thread_pool_t 
*self as a parameter. Those are instance methods, not static.




Another thing is that the initialize/terminate may not be needed. I put 
it there for now in case some thing I have overlooked.


Cheers,
Henry

Re: API Proposal for apr_thread_pool

[Henry Jen]

William A. Rowe, Jr. wrote:

Henry Jen wrote:

Hi,

We would like to implement thread pool capability, and think it might 
be a good addition to apr-util or apr.


Interesting!

I don't see where your thread join occurs to reap any terminated threads,
that is, how your model handles threads that have decided they are 'done'.



The thread remains in the pool until the thread_pool tells the thread to 
stop and that's where join will happen.


One place is apr_thread_pool_stop_unused_threads.


APR_DECLARE(int) apr_thread_pool_get_num_unused_threads(void);


AIUI the style apr_thread_pool_unused_threads_num_get() would be 
appropriate

(_num seems awkward though, I personally prefer _count, or nothing).



I am OK with the naming convention as long as there is one. :-)


/**
 * Stop all unused threads. Ignore the maximum number of idling threads.
 * @return The total number of threads stopped.
 */
APR_DECLARE(int) apr_thread_pool_stop_unused_threads(void);


I'm a little confused in your proposal between apr_thread_pool_terminate(),
apr_thread_pool_destroy(), and this function.

Can't this be reduced to



My mistake. The last couple functions should take a apr_thread_pool_t 
*self as a parameter. Those are instance methods, not static.


This may address all the following questions.

Attached please find updated version.

Cheers,
Henry


  apr_thread_pool_shutdown()  (blocks)
  apr_thread_pool_destroy()   (same as pool cleanup)

Can you elaborate?

Final observation, your model seemed a little shortsighted, in that it 
permits
only a single thread pool.  This is great for an app, lousy for another 
library
which wishes to build upon the model.  And a group of threads might be 
shut down

independently of a second pool.

Can you refine the proposal with an apr_threadpool_t * object which 
prevents
us from adding yet another evil static singleton?  Or as an alternative, 
bind
the threadpool as an attribute of the pool itself, identifying the 
thread pool

by apr_pool_t *?

Bill



/* Copyright 2000-2005 The Apache Software Foundation or its licensors, as
 * applicable.
 *
 * Licensed under the Apache License, Version 2.0 (the License);
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an AS IS BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#ifdef APR_THREAD_POOL_H
#define APR_THREAD_POOL_H

/**
 * @file apr_thread_pool.h
 * @brief APR Thread Pool Library

 * @remarks This library implements a thread pool using apr_thread_t. A thread
 * pool is a set of threads that can be created in advance or on demand until a
 * maximum number. When a task is scheduled, the thread pool will find an idle
 * thread to handle the task. In case all existing threads are busy, the pool
 * will try to create a new thread to serve the task if the maximum number has
 * not been reached. Otherwise, the task will be put into a queue based on
 * priority, which can be valued from 0 to 255, with higher value been served
 * first. In case there are tasks with the same priority, the new task is put at
 * the top or the bottom depeneds on which function is used to put the task.
 *
 * @remarks There may be the case that a thread pool can use up the maximum
 * number of threads at peak load, but having those threads idle afterwards. A
 * maximum number of idle threads can be set so that extra idling threads will
 * be terminated to save system resrouces. 
 */

#ifdef __cplusplus
extern C {
#if 0
};
#endif
#endif /* __cplusplus */

/** Opaque Thread Pool structure. */
typedef struct _apr_thread_pool apr_thread_pool_t;

/**
 * The prototype for any APR thread pool task functions.
 * @param apr_thread_t the apr_thread_t structure for the thread is executing the task
 * @param void * a pointer to the user data passed in when schedule the task
 */
typedef void* (APR_THREAD_FUNC *apr_thread_pool_task_t) (apr_thread_t *, void *)

#define APR_THREAD_TASK_PRIORITY_LOWEST 0
#define APR_THREAD_TASK_PRIORITY_LOW 63
#define APR_THREAD_TASK_PRIORITY_NORMAL 127
#define APR_THREAD_TASK_PRIORITY_HIGH 191
#define APR_THREAD_TASK_PRIORITY_HIGHEST 255

/**
 * Setup all of the internal structures required to use thread pools
 */
APR_DECLARE(apr_status_t) apr_thread_pool_init(void);

/**
 * Tear down all of the internal structures required to use pools
 */
APR_DECLARE(void) apr_thread_pool_terminate(void);

/**
 * Create a thread pool
 * @param pool The pool to use
 * @param init_threads The number of threads to be created initially, the number
 * will also be used as the initial value for maximum number of idle threads.
 * @param max_threads The maximum number of threads that can

API Proposal for apr_thread_pool

[Henry Jen]

Hi,

We would like to implement thread pool capability, and think it might be 
a good addition to apr-util or apr.


I have defined a header file and is going to implement it, if you can 
review it and give some feedback, it is much appreciated.


Cheers,
Henry
/* Copyright 2000-2005 The Apache Software Foundation or its licensors, as
 * applicable.
 *
 * Licensed under the Apache License, Version 2.0 (the License);
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an AS IS BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#ifdef APR_THREAD_POOL_H
#define APR_THREAD_POOL_H

/**
 * @file apr_thread_pool.h
 * @brief APR Thread Pool Library

 * @remarks This library implements a thread pool using apr_thread_t. A thread
 * pool is a set of threads that can be created in advance or on demand until a
 * maximum number. When a task is scheduled, the thread pool will find an idle
 * thread to handle the task. In case all existing threads are busy, the pool
 * will try to create a new thread to serve the task if the maximum number has
 * not been reached. Otherwise, the task will be put into a queue based on
 * priority, which can be valued from 0 to 255, with higher value been served
 * first. In case there are tasks with the same priority, the new task is put at
 * the top or the bottom depeneds on which function is used to put the task.
 *
 * @remarks There may be the case that a thread pool can use up the maximum
 * number of threads at peak load, but having those threads idle afterwards. A
 * maximum number of idle threads can be set so that extra idling threads will
 * be terminated to save system resrouces. 
 */

#ifdef __cplusplus
extern C {
#if 0
};
#endif
#endif /* __cplusplus */

/** Opaque Thread Pool structure. */
typedef struct _apr_thread_pool apr_thread_pool_t;

/**
 * The prototype for any APR thread pool task functions.
 * @param apr_thread_t the apr_thread_t structure for the thread is executing the task
 * @param void * a pointer to the user data passed in when schedule the task
 */
typedef void* (APR_THREAD_FUNC *apr_thread_pool_task_t) (apr_thread_t *, void *)

#define APR_THREAD_TASK_PRIORITY_LOWEST 0
#define APR_THREAD_TASK_PRIORITY_LOW 63
#define APR_THREAD_TASK_PRIORITY_NORMAL 127
#define APR_THREAD_TASK_PRIORITY_HIGH 191
#define APR_THREAD_TASK_PRIORITY_HIGHEST 255

/**
 * Setup all of the internal structures required to use thread pools
 */
APR_DECLARE(apr_status_t) apr_thread_pool_init(void);

/**
 * Tear down all of the internal structures required to use pools
 */
APR_DECLARE(void) apr_thread_pool_terminate(void);

/**
 * Create a thread pool
 * @param pool The pool to use
 * @param init_threads The number of threads to be created initially, the number
 * will also be used as the initial value for maximum number of idle threads.
 * @param max_threads The maximum number of threads that can be created
 * @param err Receive the error code, can be NULL if not needed.
 * @return The thread pool. NULL if failed to create the thread pool. Put the
 * error code in the err parameter if it is not NULL. 
 */
APR_DECLARE(apr_thread_pool_t*) apr_thread_pool_create(apr_pool_t *pool,
   int init_threads, 
   int max_threads, 
   apr_status_t *err);

/**
 * Destroy the thread pool and stop all the threads
 * @return APR_SUCCESS if all threads are stopped.
 */
APR_DECLARE(apr_status_t) apr_thread_pool_destroy(apr_thread_pool_t *self);

/**
 * Schedule a task to the bottom of the tasks of same priority.
 * @param self The thread pool
 * @param task The task function
 * @param param The parameter for the task function
 * @param priority The priority of the task.
 * @return APR_SUCCESS if the task had been scheduled successfully
 */
APR_DECLARE(apr_status_t) apr_thread_pool_push(apr_thread_pool_t *self,
   apr_thread_pool_task_t *task, 
   void *param, 
   apr_byte_t priority);

/**
 * Schedule a task to the top of the tasks of same priority.
 * @param self The thread pool
 * @param task The task function
 * @param param The parameter for the task function
 * @param priority The priority of the task.
 * @return APR_SUCCESS if the task had been scheduled successfully
 */
APR_DECLARE(apr_status_t) apr_thread_pool_top(apr_thread_pool_t *self, 
  apr_thread_pool_task_t *task, 
  

Makefile.in does not take external LIBS LDFLAGS

[Henry Jen]

Hi,

Specifying LIBS/LDFLAGS is not working with configure. For example,

$ LDFLAGS='-R/usr/sfw' ./configure --with-apr=../apr \
--with-expat=/usr/sfw

will not have the -R flag in the Makefile.

Attached patch fix this. I wonder this is a reason for not having 
LIBS/LDFLAGS in purpose as this fix is too easy, please let me know the 
reason that is the case. :-)


Cheers,
Henry
Index: Makefile.in
===
--- Makefile.in	(revision 390523)
+++ Makefile.in	(working copy)
@@ -11,8 +11,8 @@
 VPATH = @srcdir@
 
 INCLUDES = @APRUTIL_PRIV_INCLUDES@ @APR_INCLUDES@ @APRUTIL_INCLUDES@
-APRUTIL_LDFLAGS = @APRUTIL_LDFLAGS@
-APRUTIL_LIBS = @APRUTIL_LIBS@
+APRUTIL_LDFLAGS = @APRUTIL_LDFLAGS@ @LDFLAGS@
+APRUTIL_LIBS = @APRUTIL_LIBS@ @LIBS@
 
 TARGET_LIB = [EMAIL PROTECTED]@.la
 INSTALL_SUBDIRS = @APR_ICONV_DIR@ @APR_XML_DIR@

Patch for checking expat on Solaris

[Henry Jen]

Hi,

Attached patch allows to build apr-util with system provided libexpat on 
OpenSolaris.


On the machine, there is a expat.h and libexpat.so.* in /usr/sfw/include 
and /usr/sfw/lib. But there is no .a or .la file.


It should not be required to have a .la file, not to mention a .a file. 
.a is, afaik, an archive file for static link.


The attached patch check only the header file, and add the -L and -R 
flags accordingly.


I believe this should also applicable to the FreeBSD system, in that 
case, we may combine the FreeBSD and Solaris section added in this patch.


Cheers,
Henry
Index: build/apu-conf.m4
===
--- build/apu-conf.m4	(revision 390523)
+++ build/apu-conf.m4	(working copy)
@@ -84,6 +84,12 @@
 expat_include_dir=$1/include
 expat_ldflags=-L$1/lib
 expat_libs=-lexpat
+  elif test -r $1/include/expat.h; then 
+dnl Expat 1.95.* installation (without libtool)
+dnl Solaris
+expat_include_dir=$1/include
+expat_ldflags=-L$1/lib -R$1/lib
+expat_libs=-lexpat
   elif test -r $1/xmlparse.h; then
 dnl maybe an expat-lite. use this dir for both includes and libs
 expat_include_dir=$1

Re: [PATCH] include needed header files in apr_dbd.h

[Henry Jen]

Henry Jen wrote:

Hi,

When include apr_dbd.h without apu.h  apr_pools.h header first, 
compiler generate errors.


This is minor issue, but should be fixed, I think. :-)



Can someone take a minute commit this simple patch?

Cheers,
Henry




Index: include/apr_dbd.h
===
--- include/apr_dbd.h   (revision 377888)
+++ include/apr_dbd.h   (working copy)
@@ -21,6 +21,9 @@
 #ifndef APR_DBD_H
 #define APR_DBD_H
 
+#include apu.h

+#include apr_pools.h
+
 #ifdef __cplusplus
 extern C {
 #endif

[PATCH] dbd.m4: use LDFLAGS instead of LIBS is required on Solaris

[Henry Jen]

Hi,

AC_CHECK_LIB is performed as $CC $CFLAGS $LDFLAGS -llib $LIBS conftest.c

With Sun Studio, -L needs to be in front of -l flag, which makes sense 
when you need control on which lib to be linked with.


In dbd.m4, using LIBS to define the -L flag, which should really be 
LDFLAGS, the attached patch does just that.


Please help to review and commit the patch. Much appreciated.

Cheers,
Henry
Index: build/dbd.m4
===
--- build/dbd.m4	(revision 390221)
+++ build/dbd.m4	(working copy)
@@ -40,7 +40,7 @@
   apu_have_pgsql=0
 else
   CPPFLAGS=-I$withval/include
-  LIBS=-L$withval/lib 
+  LDFLAGS=-L$withval/lib 
 
   AC_MSG_NOTICE(checking for pgsql in $withval)
   AC_CHECK_HEADER(libpq-fe.h, AC_CHECK_LIB(pq, PQsendQueryPrepared, [apu_have_pgsql=1]))
@@ -88,7 +88,7 @@
   apu_have_mysql=0
 else
   CPPFLAGS=-I$withval/include
-  LIBS=-L$withval/lib 
+  LDFLAGS=-L$withval/lib 
 
   AC_MSG_NOTICE(checking for mysql in $withval)
   AC_CHECK_HEADER(mysql.h, AC_CHECK_LIB(mysqlclient_r, mysql_init, [apu_have_mysql=1]))
@@ -133,7 +133,7 @@
   apu_have_sqlite3=0
 else
   CPPFLAGS=-I$withval/include
-  LIBS=-L$withval/lib 
+  LDFLAGS=-L$withval/lib 
 
   AC_MSG_NOTICE(checking for sqlite3 in $withval)
   AC_CHECK_HEADER(sqlite3.h, AC_CHECK_LIB(sqlite3, sqlite3_open, [apu_have_sqlite3=1]))
@@ -170,7 +170,7 @@
   apu_have_sqlite2=0
 else
   CPPFLAGS=-I$withval/include
-  LIBS=-L$withval/lib 
+  LDFLAGS=-L$withval/lib 
 
   AC_MSG_NOTICE(checking for sqlite2 in $withval)
   AC_CHECK_HEADER(sqlite.h, AC_CHECK_LIB(sqlite, sqlite_open, [apu_have_sqlite2=1]))
@@ -207,7 +207,7 @@
   apu_have_oracle=0
 else
   CPPFLAGS=-I$withval/rdbms/demo -I$withval/rdbms/public
-  LIBS=-L$withval/lib 
+  LDFLAGS=-L$withval/lib 
 
   AC_MSG_NOTICE(checking for oracle in $withval)
   AC_CHECK_HEADER(oci.h, AC_CHECK_LIB(clntsh, OCIEnvCreate, [apu_have_oracle=1]))

[PATCH] include needed header files in apr_dbd.h

[Henry Jen]

Hi,

When include apr_dbd.h without apu.h  apr_pools.h header first, 
compiler generate errors.


This is minor issue, but should be fixed, I think. :-)

Cheers,
Henry
Index: include/apr_dbd.h
===
--- include/apr_dbd.h	(revision 377888)
+++ include/apr_dbd.h	(working copy)
@@ -21,6 +21,9 @@
 #ifndef APR_DBD_H
 #define APR_DBD_H
 
+#include apu.h
+#include apr_pools.h
+
 #ifdef __cplusplus
 extern C {
 #endif

Re: apr_pool_terminate

[Henry Jen]

Branko Čibej wrote:
Grrr.Sometimes I wish there was a cancel post option for e-mail as 
well as news.


Just ignore me, I'm going through my stupid phase again.



So, what about it? :-)

Cheers,
Henry




Henry Jen wrote:


Henry Jen wrote:


Hi,

The debug version of apr_poor_terminate does not support nested 
initialization as regular version, is this a bug or on purpose?


I guess it is a bug, but want to hear second opinion. :-)




I guess no one would care if there is no patch available. So here is 
a patch. :-)




Yeah. Pity it's wrong...


Index: memory/unix/apr_pools.c
===
--- memory/unix/apr_pools.c(revision 230894)
+++ memory/unix/apr_pools.c(working copy)
@@ -1264,7 +1264,8 @@
if (!apr_pools_initialized)
return;

-apr_pools_initialized = 0;
+if (--apr_pools_initialized)
+return;
 


Take a good look at what happens just before the line you changed.



-- Brane

Re: apr_pool_terminate

[Henry Jen]

Henry Jen wrote:

Hi,

The debug version of apr_poor_terminate does not support nested 
initialization as regular version, is this a bug or on purpose?


I guess it is a bug, but want to hear second opinion. :-)




I guess no one would care if there is no patch available. So here is a 
patch. :-)


Cheers,
Henry
Index: memory/unix/apr_pools.c
===
--- memory/unix/apr_pools.c	(revision 230894)
+++ memory/unix/apr_pools.c	(working copy)
@@ -1264,7 +1264,8 @@
 if (!apr_pools_initialized)
 return;
 
-apr_pools_initialized = 0;
+if (--apr_pools_initialized)
+return;
 
 apr_pool_destroy(global_pool); /* This will also destroy the mutex */
 global_pool = NULL;

apr_pool_terminate

[Henry Jen]

Hi,

The debug version of apr_poor_terminate does not support nested 
initialization as regular version, is this a bug or on purpose?


I guess it is a bug, but want to hear second opinion. :-)

Cheers,
Henry

Re: apr_dbd

[Henry Jen]

John Vandenberg wrote:

On 8/4/05, Nick Kew [EMAIL PROTECTED] wrote:

Hi Nick,



As indicated in my post of a few weeks ago, I'd like to support typed
data in apr_dbd.  I posted a patch that would change (and break) the
API.  Since apr_dbd hasn't yet featured in a release version, there was
no absolute reason not to.



I am not familiar with the existing purpose behind the apr_dbd
interface, but it strikes me as odd that apr needs to incorporate a
complex DB abstraction layer; a simple string based layer, like libsdb
(lgpl), would suffice for most needs.

Also, if a complex layer is needed, what is the advantage of building
a new one for apr; could libdbi (libdbi) or sqlrelay be used instead?




All you mentioned is either LGPL or GPL, which are not acceptable for 
projects want to be using Apache or BSD-like license.


Another goal is to make sure portability and light. sqlrelay has 
additional dependency and not available for Windows.


My 2 cents.

Cheers,
Henry

Re: Recursive mutex

[Henry Jen]

Joe Orton wrote:

On Thu, Jul 21, 2005 at 11:37:29AM -0700, Henry Jen wrote:

I just looked into SVN at 0.9.4(thanks Joe for pointing me there), 
that's pretty much what I planned to do. The trylock does not seem to be 
right though.


However, I would like to know what's wrong with that implementation and 
try to figure out how it can be fixed.



http://mail-archives.apache.org/mod_mbox/apr-dev/200307.mbox/[EMAIL PROTECTED]

and the rest of that thread probably help, I think those comments apply 
to the code as it was before it was removed.


I was finally motivated to throw out the old code and replace it with 
use of POSIX recursive mutexes since doing so fixed segfaults in httpd 
worker MPM regression tests on x86_64 with pool-debug enabled (the 
pool-debug code is the only place inside apr/httpd where the NESTED flag 
is used).  I don't know if those segfaults were directly attributable to 
any of the issues I highlighted in the old code, however, just to keep 
you on your toes ;)




Thanks, that thread helps a lot.

Our user decided to ask the Linux vendor to support recursive mutex, so 
I don't need to worry about that now. :-)


Thanks again for all the pointers.

Cheers,
Henry

Re: [VOTE] APR 1.2.0

[Henry Jen]

Paul Querna wrote:

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Good Afternoon,

APR 1.2.0 is ready for testing and voting for release.

Download in your favorite archive format at:
http://people.apache.org/~pquerna/dev/apr-1.2.0/

apr-1.2.0.tar.bz2: FE 46 07 E9 28 77 4E 80  10 6F 71 19 26 18 8A 5D
apr-1.2.0.tar.gz: 56 7A 8F 00 75 BD DD 00  60 A4 F0 AD DC D8 BF DA
apr-1.2.0.zip: 96 00 50 6F 5C EC 0F 2B  59 ED 9A ED 39 A2 80 B5


There is no corresponding APR-Util tag or release at this time.
APR-Util will get their own release when they are ready.

- -Paul


-1 for Win32, the condvars deadlock is a serious bug. I knew this is not 
news, but as the patch had been available for quite a while, is it 
possible to get it fixed?


Cheers,
Henry

Re: [VOTE] APR 1.2.0

[Henry Jen]

Paul Querna wrote:

Henry Jen wrote:



-1 for Win32, the condvars deadlock is a serious bug. I knew this is not
news, but as the patch had been available for quite a while, is it
possible to get it fixed?



No.

I will not commit such a platform specific patch.  Anyone who actually
compiles APR on win32 that wants to do it?



I tried to convince our team members to use/contribute to apr as much as 
we can, the recent apr_dbd_sqlite3 driver by Rick Keiner is one example.


Cannot make progress on such a serious bug will make me much harder to 
pitch the idea of a broader collaboration between jxta-c and apr project.


Please someone looks into the patch.

Thanks,
Henry

Re: [VOTE] APR 1.2.0

[Henry Jen]

E Holyat wrote:

Here is a patch for win32 that has been tested extensively for a few
 months now.  I submitted it to bugzilla

The previous patch addressed only the unlock being called more than
once.

This attachment avoids race conditions that the previous patch
doesn't. This patch also fixes the multiple calls to unlock. This
patch also consolidates the the duplicate efforts in
apr_thread_cond_wait and apr_thread_cond_timedwait


Seems like there are couple patches around, here is the one I submitted
on Jun 02, this patch is used by jxta-c project.


Hi,

We encountered the issue as reported in Bug 
27654(http://issues.apache.org/bugzilla/show_bug.cgi?id=27654), too

bad I did not check the issue database earlier until I figured it out
after 3 days.

The patch in the issues database should be working, is there a reason
 for not committing it?

Anyway, I have created another patch(see attachment) to address the
same issue and which would also fix bug 34336 as it totally removing
the cond-mutex. I believe using the mutex passed in to ensure mutual
access should be sufficient.

Unless the different threads can use different mutex for the same
cond, but I don't see that as a valid usage and nor can I think of a
use case for that.

Please review it, comments are welcome. I would like to see the patch
be committed to head and also back port to 0.9.x release. The patch
had been tested for both apr-0.9.6 and SVN trunk with 'patch -Np0' in
the apr folder.
--- include/arch/win32/apr_arch_thread_cond.orig_096.h	2005-02-04 12:36:31.0 -0800
+++ include/arch/win32/apr_arch_thread_cond.h	2005-06-02 18:00:01.25000 -0700
@@ -22,7 +22,6 @@
 struct apr_thread_cond_t {
 apr_pool_t *pool;
 HANDLE event;
-HANDLE mutex;
 int signal_all;
 int num_waiting;
 int signalled;
--- locks/win32/thread_cond.orig_096.c	2005-06-02 22:27:51.265625000 -0700
+++ locks/win32/thread_cond.c	2005-06-02 22:34:06.078125000 -0700
@@ -25,7 +25,6 @@
 static apr_status_t thread_cond_cleanup(void *data)
 {
 apr_thread_cond_t *cond = data;
-CloseHandle(cond-mutex);
 CloseHandle(cond-event);
 return APR_SUCCESS;
 }
@@ -36,7 +35,6 @@
 *cond = apr_palloc(pool, sizeof(**cond));
 (*cond)-pool = pool;
 (*cond)-event = CreateEvent(NULL, TRUE, FALSE, NULL);
-(*cond)-mutex = CreateMutex(NULL, FALSE, NULL);
 (*cond)-signal_all = 0;
 (*cond)-num_waiting = 0;
 return APR_SUCCESS;
@@ -48,20 +46,14 @@
 DWORD res;
 
 while (1) {
-res = WaitForSingleObject(cond-mutex, INFINITE);
-if (res != WAIT_OBJECT_0) {
-return apr_get_os_error();
-}
 cond-num_waiting++;
-ReleaseMutex(cond-mutex);
 
 apr_thread_mutex_unlock(mutex);
 res = WaitForSingleObject(cond-event, INFINITE);
+apr_thread_mutex_lock(mutex);
 cond-num_waiting--;
 if (res != WAIT_OBJECT_0) {
-apr_status_t rv = apr_get_os_error();
-ReleaseMutex(cond-mutex);
-return rv;
+return apr_get_os_error();
 }
 if (cond-signal_all) {
 if (cond-num_waiting == 0) {
@@ -74,9 +66,7 @@
 ResetEvent(cond-event);
 break;
 }
-ReleaseMutex(cond-mutex);
 }
-apr_thread_mutex_lock(mutex);
 return APR_SUCCESS;
 }
 
@@ -88,23 +78,14 @@
 DWORD timeout_ms = (DWORD) apr_time_as_msec(timeout);
 
 while (1) {
-res = WaitForSingleObject(cond-mutex, timeout_ms);
-if (res != WAIT_OBJECT_0) {
-if (res == WAIT_TIMEOUT) {
-return APR_TIMEUP;
-}
-return apr_get_os_error();
-}
 cond-num_waiting++;
-ReleaseMutex(cond-mutex);
 
 apr_thread_mutex_unlock(mutex);
 res = WaitForSingleObject(cond-event, timeout_ms);
+apr_thread_mutex_lock(mutex);
 cond-num_waiting--;
 if (res != WAIT_OBJECT_0) {
 apr_status_t rv = apr_get_os_error();
-ReleaseMutex(cond-mutex);
-apr_thread_mutex_lock(mutex);
 if (res == WAIT_TIMEOUT) {
 return APR_TIMEUP;
 }
@@ -121,9 +102,7 @@
 ResetEvent(cond-event);
 break;
 }
-ReleaseMutex(cond-mutex);
 }
-apr_thread_mutex_lock(mutex);
 return APR_SUCCESS;
 }
 
@@ -132,16 +111,11 @@
 apr_status_t rv = APR_SUCCESS;
 DWORD res;
 
-res = WaitForSingleObject(cond-mutex, INFINITE);
-if (res != WAIT_OBJECT_0) {
-return apr_get_os_error();
-}
 cond-signalled = 1;
 res = SetEvent(cond-event);
 if (res == 0) {
 rv = apr_get_os_error();
 }
-ReleaseMutex(cond-mutex);
 return rv;
 }
 
@@ -150,17 +124,12 @@
 apr_status_t rv = APR_SUCCESS;
 DWORD res;
 
-res = WaitForSingleObject(cond-mutex, INFINITE);
-if (res != WAIT_OBJECT_0) {
-return apr_get_os_error();
-}
 cond-signalled = 1;
 

Re: [VOTE] APR 1.2.0

[Henry Jen]

Ed Holyat wrote:

I like that you were able to get rid of an extra mutex.

Their is some problems though.

There is no way to safely signal the condition, a free read and/or 
write on signalled and signal_all introduces multiple race

conditions.  In order for this operation to be safe, it would have to
be made atomic.(not a bad idea)

But, this will never eliminate the race in checking the following
area of code. Without mutexing the if block, It is possible for
signal_all to be false, we fall into the else if.. another CPU
executes the apr_thread_cond_broadcast we will ResetEvent prematurely
if num_waiting  0

if (cond-signal_all) { if (cond-num_waiting == 0) { 
ResetEvent(cond-event); } done=1; } else if (cond-signalled) { 
cond-signalled = 0; ResetEvent(cond-event); done=1; }





As I explained, we rely on the associated mutex to protect those
variables and that should be effcient.

According to the doc for apr_thread_cond_signal:

Although it is not required, if predictable scheduling is desired, 
that mutex must be locked while calling this function.


Also, on *nix systems, check out the man page of pthread_cond_broadcast
says the same thing.

Thanks,
Henry

Re: [VOTE] APR 1.2.0

[Henry Jen]

E Holyat wrote:
  I don't use the broadcast functionality, but, it looks like that is 
broken too.  signal_all is never reset to 0 after the broadcast function 
sets it to 1.  Here is an untested fix
 
   cond-num_waiting--;/*we have the lock(s)*/

   if (cond-signal_all) {
   if (cond-num_waiting == 0) {
+  cond-signal_all=0;
  ResetEvent(cond-event);
   }
   done=1;
   }
   else if (cond-signalled) {
   cond-signalled = 0;
   ResetEvent(cond-event);
   done=1;
   }



Good catch, you also need to reset cond-signalled.
I adapted your changes on the else if also the static INLINE internal 
function and created a new patch.


Cheers,
Henry
Index: locks/win32/thread_cond.c
===
--- locks/win32/thread_cond.c	(revision 219961)
+++ locks/win32/thread_cond.c	(working copy)
@@ -25,7 +25,6 @@
 static apr_status_t thread_cond_cleanup(void *data)
 {
 apr_thread_cond_t *cond = data;
-CloseHandle(cond-mutex);
 CloseHandle(cond-event);
 return APR_SUCCESS;
 }
@@ -36,95 +35,61 @@
 *cond = apr_palloc(pool, sizeof(**cond));
 (*cond)-pool = pool;
 (*cond)-event = CreateEvent(NULL, TRUE, FALSE, NULL);
-(*cond)-mutex = CreateMutex(NULL, FALSE, NULL);
 (*cond)-signal_all = 0;
 (*cond)-num_waiting = 0;
 return APR_SUCCESS;
 }
 
-APR_DECLARE(apr_status_t) apr_thread_cond_wait(apr_thread_cond_t *cond,
-   apr_thread_mutex_t *mutex)
+static APR_INLINE apr_status_t _thread_cond_timedwait(apr_thread_cond_t *cond,
+  apr_thread_mutex_t *mutex,
+  DWORD timeout_ms )
 {
 DWORD res;
 
 while (1) {
-res = WaitForSingleObject(cond-mutex, INFINITE);
-if (res != WAIT_OBJECT_0) {
-return apr_get_os_error();
-}
 cond-num_waiting++;
-ReleaseMutex(cond-mutex);
 
 apr_thread_mutex_unlock(mutex);
-res = WaitForSingleObject(cond-event, INFINITE);
+res = WaitForSingleObject(cond-event, timeout_ms);
+apr_thread_mutex_lock(mutex);
 cond-num_waiting--;
 if (res != WAIT_OBJECT_0) {
 apr_status_t rv = apr_get_os_error();
-ReleaseMutex(cond-mutex);
-return rv;
+if (res == WAIT_TIMEOUT) {
+return APR_TIMEUP;
+}
+return apr_get_os_error();
 }
 if (cond-signal_all) {
 if (cond-num_waiting == 0) {
+cond-signal_all = 0;
+cond-signalled = 0;
 ResetEvent(cond-event);
 }
 break;
 }
-if (cond-signalled) {
+else if (cond-signalled) {
 cond-signalled = 0;
 ResetEvent(cond-event);
 break;
 }
-ReleaseMutex(cond-mutex);
 }
-apr_thread_mutex_lock(mutex);
 return APR_SUCCESS;
 }
 
+APR_DECLARE(apr_status_t) apr_thread_cond_wait(apr_thread_cond_t *cond,
+   apr_thread_mutex_t *mutex)
+{
+return _thread_cond_timedwait(cond, mutex, INFINITE);
+}
+
 APR_DECLARE(apr_status_t) apr_thread_cond_timedwait(apr_thread_cond_t *cond,
 apr_thread_mutex_t *mutex,
 apr_interval_time_t timeout)
 {
-DWORD res;
 DWORD timeout_ms = (DWORD) apr_time_as_msec(timeout);
 
-while (1) {
-res = WaitForSingleObject(cond-mutex, timeout_ms);
-if (res != WAIT_OBJECT_0) {
-if (res == WAIT_TIMEOUT) {
-return APR_TIMEUP;
-}
-return apr_get_os_error();
-}
-cond-num_waiting++;
-ReleaseMutex(cond-mutex);
-
-apr_thread_mutex_unlock(mutex);
-res = WaitForSingleObject(cond-event, timeout_ms);
-cond-num_waiting--;
-if (res != WAIT_OBJECT_0) {
-apr_status_t rv = apr_get_os_error();
-ReleaseMutex(cond-mutex);
-apr_thread_mutex_lock(mutex);
-if (res == WAIT_TIMEOUT) {
-return APR_TIMEUP;
-}
-return apr_get_os_error();
-}
-if (cond-signal_all) {
-if (cond-num_waiting == 0) {
-ResetEvent(cond-event);
-}
-break;
-}
-if (cond-signalled) {
-cond-signalled = 0;
-ResetEvent(cond-event);
-break;
-}
-ReleaseMutex(cond-mutex);
-}
-apr_thread_mutex_lock(mutex);
-return APR_SUCCESS;
+return _thread_cond_timedwait(cond, mutex, timeout_ms);
 }
 
 APR_DECLARE(apr_status_t) apr_thread_cond_signal(apr_thread_cond_t *cond)
@@ -132,16 +97,11 @@
 apr_status_t rv = APR_SUCCESS;
 DWORD res;
 
-res = WaitForSingleObject(cond-mutex, INFINITE);
-