You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Aaron Bannert <aa...@ebuilt.com> on 2001/07/20 19:30:53 UTC
[PATCH] APR thread updates and associated httpd-2.0 changes
I submitted most of this patch last weekend. Since then I've fixed
a small bug in the apr_thread_exit() patch I made (I could really
use APR commit access for the little things like this :). I haven't
heard any objections (Ryan ;), so I assume these changes were acceptable.
I've also brought my httpd-2.0 tree up to date with the APR patch.
(Pretty minor changes. Please verify.)
As a reminder, here are the things this fixes/changes:
- fixes apr_thread_exit(), it wasn't actually passing the return status back.
- changes apr_thread_start_t (worker's function prototype) to take the
parameter apr_thread_param_t, which contains the thread pointer and
the opaque data pointer.
- adds a new test in test/testthread.c to make sure apr_thread_exit() is
working.
(and the new parts for httpd-2.0)
- updates httpd-2.0 to work with the above changes. changes the worker
functions in the threaded mpm and perchild mpm.
enjoy,
-aaron
Index: server/mpm/perchild/perchild.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/mpm/perchild/perchild.c,v
retrieving revision 1.71
diff -u -r1.71 perchild.c
--- server/mpm/perchild/perchild.c 2001/07/18 20:45:35 1.71
+++ server/mpm/perchild/perchild.c 2001/07/20 17:11:30
@@ -512,7 +512,7 @@
}
}
-static void *worker_thread(void *);
+static void *worker_thread(apr_thread_param_t *);
/* Starts a thread as long as we're below max_threads */
static int start_thread(void)
@@ -579,8 +579,9 @@
/* idle_thread_count should be incremented before starting a worker_thread */
-static void *worker_thread(void *arg)
+static void *worker_thread(apr_thread_param_t *param)
{
+ apr_thread_t *thread = param->t;
apr_socket_t *csd = NULL;
apr_pool_t *tpool; /* Pool for this thread */
apr_pool_t *ptrans; /* Pool for per-transaction stuff */
@@ -588,7 +589,7 @@
int srv;
int curr_pollfd, last_pollfd = 0;
int thread_just_started = 1;
- int thread_num = *((int *) arg);
+ int thread_num = *((int *) param->data);
long conn_id = child_num * HARD_THREAD_LIMIT + thread_num;
apr_pollfd_t *pollset;
int n;
@@ -789,6 +790,7 @@
}
apr_lock_release(worker_thread_count_mutex);
+ apr_thread_exit(thread, NULL);
return NULL;
}
Index: server/mpm/threaded/threaded.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/mpm/threaded/threaded.c,v
retrieving revision 1.46
diff -u -r1.46 threaded.c
--- server/mpm/threaded/threaded.c 2001/07/18 20:45:36 1.46
+++ server/mpm/threaded/threaded.c 2001/07/20 17:11:32
@@ -520,9 +520,10 @@
apr_lock_release(pipe_of_death_mutex);
}
-static void * worker_thread(void * dummy)
+static void * worker_thread(apr_thread_param_t *param)
{
- proc_info * ti = dummy;
+ apr_thread_t *thread = param->t;
+ proc_info * ti = param->data;
int process_slot = ti->pid;
int thread_slot = ti->tid;
apr_pool_t *tpool = ti->tpool;
@@ -658,6 +659,7 @@
}
apr_lock_release(worker_thread_count_mutex);
+ apr_thread_exit(thread, NULL);
return NULL;
}
@@ -671,9 +673,10 @@
return 0;
}
-static void *start_threads(void * dummy)
+static void *start_threads(apr_thread_param_t *param)
{
- thread_starter *ts = dummy;
+ apr_thread_t *thread = param->t;
+ thread_starter *ts = param->data;
apr_thread_t **threads = ts->threads;
apr_threadattr_t *thread_attr = ts->threadattr;
int child_num_arg = ts->child_num_arg;
@@ -733,6 +736,7 @@
* "life_status" is almost right, but it's in the worker's structure, and
* the name could be clearer. gla
*/
+ apr_thread_exit(thread, NULL);
return NULL;
}
Index: srclib/apr/include/apr_thread_proc.h
===================================================================
RCS file: /home/cvspublic/apr/include/apr_thread_proc.h,v
retrieving revision 1.65
diff -u -r1.65 apr_thread_proc.h
--- srclib/apr/include/apr_thread_proc.h 2001/06/06 18:11:06 1.65
+++ srclib/apr/include/apr_thread_proc.h 2001/07/20 17:11:32
@@ -125,7 +125,13 @@
typedef struct apr_other_child_rec_t apr_other_child_rec_t;
#endif /* APR_HAS_OTHER_CHILD */
-typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *);
+struct apr_thread_param_t {
+ apr_thread_t *t;
+ void *data;
+};
+typedef struct apr_thread_param_t apr_thread_param_t;
+
+typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_param_t *);
enum kill_conditions {
kill_never, /* process is never sent any signals */
Index: srclib/apr/test/testthread.c
===================================================================
RCS file: /home/cvspublic/apr/test/testthread.c,v
retrieving revision 1.17
diff -u -r1.17 testthread.c
--- srclib/apr/test/testthread.c 2001/03/14 15:56:44 1.17
+++ srclib/apr/test/testthread.c 2001/07/20 17:11:33
@@ -72,57 +72,66 @@
}
#else /* !APR_HAS_THREADS */
-void * APR_THREAD_FUNC thread_func1(void *data);
-void * APR_THREAD_FUNC thread_func2(void *data);
-void * APR_THREAD_FUNC thread_func3(void *data);
-void * APR_THREAD_FUNC thread_func4(void *data);
+void * APR_THREAD_FUNC thread_func1(apr_thread_param_t *thrparm);
+void * APR_THREAD_FUNC thread_func2(apr_thread_param_t *thrparm);
+void * APR_THREAD_FUNC thread_func3(apr_thread_param_t *thrparm);
+void * APR_THREAD_FUNC thread_func4(apr_thread_param_t *thrparm);
apr_lock_t *thread_lock;
apr_pool_t *context;
int x = 0;
+apr_status_t exit_ret_val = 123; /* just some made up number to check on later */
-void * APR_THREAD_FUNC thread_func1(void *data)
+void * APR_THREAD_FUNC thread_func1(apr_thread_param_t *thrparm)
{
int i;
+ apr_thread_t *thread = thrparm->t;
for (i = 0; i < 10000; i++) {
apr_lock_acquire(thread_lock);
x++;
apr_lock_release(thread_lock);
}
+ apr_thread_exit(thread, &exit_ret_val);
return NULL;
}
-void * APR_THREAD_FUNC thread_func2(void *data)
+void * APR_THREAD_FUNC thread_func2(apr_thread_param_t *thrparm)
{
int i;
+ apr_thread_t *thread = thrparm->t;
for (i = 0; i < 10000; i++) {
apr_lock_acquire(thread_lock);
x++;
apr_lock_release(thread_lock);
}
+ apr_thread_exit(thread, &exit_ret_val);
return NULL;
}
-void * APR_THREAD_FUNC thread_func3(void *data)
+void * APR_THREAD_FUNC thread_func3(apr_thread_param_t *thrparm)
{
int i;
+ apr_thread_t *thread = thrparm->t;
for (i = 0; i < 10000; i++) {
apr_lock_acquire(thread_lock);
x++;
apr_lock_release(thread_lock);
}
+ apr_thread_exit(thread, &exit_ret_val);
return NULL;
}
-void * APR_THREAD_FUNC thread_func4(void *data)
+void * APR_THREAD_FUNC thread_func4(apr_thread_param_t *thrparm)
{
int i;
+ apr_thread_t *thread = thrparm->t;
for (i = 0; i < 10000; i++) {
apr_lock_acquire(thread_lock);
x++;
apr_lock_release(thread_lock);
}
+ apr_thread_exit(thread, &exit_ret_val);
return NULL;
}
@@ -172,7 +181,15 @@
apr_thread_join(&s2, t2);
apr_thread_join(&s3, t3);
apr_thread_join(&s4, t4);
- fprintf (stdout, "OK\n");
+ fprintf (stdout, "OK\n");
+
+ fprintf(stdout, "Checking thread's returned value.......");
+ if (s1 != exit_ret_val || s2 != exit_ret_val ||
+ s3 != exit_ret_val || s4 != exit_ret_val) {
+ fprintf(stderr, "Invalid return value (not expected value)\n");
+ exit(-1);
+ }
+ fprintf(stdout, "OK\n");
fprintf(stdout, "Checking if locks worked.......");
if (x != 40000) {
Index: srclib/apr/threadproc/beos/thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/beos/thread.c,v
retrieving revision 1.22
diff -u -r1.22 thread.c
--- srclib/apr/threadproc/beos/thread.c 2001/06/14 18:52:05 1.22
+++ srclib/apr/threadproc/beos/thread.c 2001/07/20 17:11:33
@@ -94,13 +94,17 @@
{
int32 temp;
apr_status_t stat;
+ struct thread_info_t *thr_info;
+ apr_thread_t *thread;
+ apr_thread_param_t *thrparm;
- (*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
- if ((*new) == NULL) {
+ thread = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
+ (*new) = thread;
+ if (thread == NULL) {
return APR_ENOMEM;
}
- (*new)->cntxt = cont;
+ thread->cntxt = cont;
/* First we create the new thread...*/
if (attr)
@@ -108,14 +112,23 @@
else
temp = B_NORMAL_PRIORITY;
- stat = apr_pool_create(&(*new)->cntxt, cont);
+ stat = apr_pool_create(&thread->cntxt, cont);
if (stat != APR_SUCCESS) {
return stat;
}
- (*new)->td = spawn_thread((thread_func)func, "apr thread", temp, data);
+ thread->cntxt = cont;
+
+ thrparm = (apr_thread_param_t *)apr_palloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+ thrparm->t = thread;
+ thrparm->data = data;
+
+ thread->td = spawn_thread((thread_func)func, "apr thread", temp, thrparm);
/* Now we try to run it...*/
- if (resume_thread((*new)->td) == B_NO_ERROR) {
+ if (resume_thread(thread->td) == B_NO_ERROR) {
return APR_SUCCESS;
}
else {
@@ -142,7 +155,11 @@
apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t *thd)
{
- if (wait_for_thread(thd->td,(void *)&retval) == B_NO_ERROR) {
+ apr_status_t *thrstat = NULL;
+
+ if (wait_for_thread(thd->td,(void *)&thrstat) == B_NO_ERROR) {
+ if (retval != NULL && thrstat != NULL)
+ *retval = *thrstat;
return APR_SUCCESS;
}
else {
Index: srclib/apr/threadproc/os2/thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/os2/thread.c,v
retrieving revision 1.22
diff -u -r1.22 thread.c
--- srclib/apr/threadproc/os2/thread.c 2001/06/16 01:27:15 1.22
+++ srclib/apr/threadproc/os2/thread.c 2001/07/20 17:11:33
@@ -94,8 +94,18 @@
static void apr_thread_begin(void *arg)
{
- apr_thread_t *thread = (apr_thread_t *)arg;
- thread->rv = thread->func(thread->data);
+ apr_thread_param_t *thrparm;
+ apr_thread_t *thread = (apr_thread_t *)arg;
+
+ thrparm = (apr_thread_param_t *)apr_palloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+
+ thrparm->t = thread;
+ thrparm->data = data;
+
+ thread->rv = thread->func(thrparm);
}
@@ -132,12 +142,23 @@
}
}
- if (thread->attr->attr & APR_THREADATTR_DETACHED)
+ if (thread->attr->attr & APR_THREADATTR_DETACHED) {
+ apr_thread_param_t *thrparm;
+
+ thrparm = (apr_thread_param_t *)apr_palloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+
+ thrparm->t = thread;
+ thrparm->data = data;
+
thread->tid = _beginthread((os2_thread_start_t)func, NULL,
- APR_THREAD_STACKSIZE, data);
- else
+ APR_THREAD_STACKSIZE, thrparm);
+ } else {
thread->tid = _beginthread(apr_thread_begin, NULL,
APR_THREAD_STACKSIZE, thread);
+ }
if (thread->tid < 0) {
return errno;
@@ -180,7 +201,8 @@
if (rc == ERROR_INVALID_THREADID)
rc = 0; /* Thread had already terminated */
- *retval = (apr_status_t)thd->rv;
+ if (retval != NULL)
+ *retval = (apr_status_t)thd->rv;
return APR_OS2_STATUS(rc);
}
Index: srclib/apr/threadproc/unix/thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/unix/thread.c,v
retrieving revision 1.39
diff -u -r1.39 thread.c
--- srclib/apr/threadproc/unix/thread.c 2001/06/14 18:52:09 1.39
+++ srclib/apr/threadproc/unix/thread.c 2001/07/20 17:11:33
@@ -122,32 +122,43 @@
{
apr_status_t stat;
pthread_attr_t *temp;
+ apr_thread_t *thread;
+ apr_thread_param_t *thrparm;
- (*new) = (apr_thread_t *)apr_pcalloc(cont, sizeof(apr_thread_t));
+ thread = (apr_thread_t *)apr_pcalloc(cont, sizeof(apr_thread_t));
+ (*new) = thread;
- if ((*new) == NULL) {
+ if (thread == NULL) {
return APR_ENOMEM;
}
- (*new)->td = (pthread_t *)apr_pcalloc(cont, sizeof(pthread_t));
+ thread->td = (pthread_t *)apr_pcalloc(cont, sizeof(pthread_t));
- if ((*new)->td == NULL) {
+ if (thread->td == NULL) {
return APR_ENOMEM;
}
- (*new)->cntxt = cont;
-
if (attr)
temp = attr->attr;
else
temp = NULL;
-
- stat = apr_pool_create(&(*new)->cntxt, cont);
+
+ thread->cntxt = cont;
+
+ stat = apr_pool_create(&thread->cntxt, cont);
if (stat != APR_SUCCESS) {
return stat;
}
+
+ thrparm = (apr_thread_param_t *)apr_pcalloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
- if ((stat = pthread_create((*new)->td, temp, func, data)) == 0) {
+ thrparm->t = thread;
+ thrparm->data = data;
+
+ if ((stat = pthread_create(thread->td, temp, (void*)func, thrparm)) == 0) {
return APR_SUCCESS;
}
else {
@@ -155,7 +166,7 @@
stat = errno;
#endif
return stat;
- }
+ }
}
apr_os_thread_t apr_os_thread_current(void)
@@ -178,8 +189,11 @@
apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t *thd)
{
apr_status_t stat;
+ apr_status_t *thrstat = NULL;
- if ((stat = pthread_join(*thd->td,(void *)&retval)) == 0) {
+ if ((stat = pthread_join(*thd->td,(void **)&thrstat)) == 0) {
+ if (retval != NULL && thrstat != NULL)
+ *retval = *thrstat;
return APR_SUCCESS;
}
else {
Index: srclib/apr/threadproc/win32/thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/win32/thread.c,v
retrieving revision 1.32
diff -u -r1.32 thread.c
--- srclib/apr/threadproc/win32/thread.c 2001/07/06 14:20:03 1.32
+++ srclib/apr/threadproc/win32/thread.c 2001/07/20 17:11:33
@@ -97,25 +97,36 @@
apr_status_t stat;
unsigned temp;
int lasterror;
+ apr_thread_t *thread;
+ apr_thread_param_t *thrparm;
- (*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
+ thread = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
+ (*new) = thread;
- if ((*new) == NULL) {
+ if (thread == NULL) {
return APR_ENOMEM;
}
- (*new)->cntxt = cont;
+ thread->cntxt = cont;
- stat = apr_pool_create(&(*new)->cntxt, cont);
+ stat = apr_pool_create(&thread->cntxt, cont);
if (stat != APR_SUCCESS) {
return stat;
}
+ thrparm = (apr_thread_param_t *)apr_palloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+
+ thrparm->t = thread;
+ thrparm->data = data;
+
/* Use 0 for Thread Stack Size, because that will default the stack to the
* same size as the calling thread.
*/
- if (((*new)->td = (HANDLE *)_beginthreadex(NULL, 0, (unsigned int (APR_THREAD_FUNC *)(void *))func,
- data, 0, &temp)) == 0) {
+ if ((thread->td = (HANDLE *)_beginthreadex(NULL, 0, (unsigned int (APR_THREAD_FUNC *)(void *))func,
+ thrparm, 0, &temp)) == 0) {
lasterror = apr_get_os_error();
return APR_EEXIST;
/* MSVC++ doc doesn't mention any additional error info
@@ -124,7 +135,7 @@
}
if (attr && attr->detach) {
- CloseHandle((*new)->td);
+ CloseHandle(thread->td);
}
return APR_SUCCESS;
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Aaron Bannert" <aa...@ebuilt.com>
Sent: Friday, July 20, 2001 11:57 PM
> On Fri, Jul 20, 2001 at 09:20:49PM -0700, Ryan Bloom wrote:
> >
> > MNSO is that the goal of
> > patches should be to change as little code as possible. The more changes there are,
> > the harder it is to review the patch. If a patch is generated, and there isn't a need for
> > a line of code to have changed, the person who created the patch should go through
> > and ask why that line was changed.
>
> Your inspection is valid, but I'm not sure it is in the best interest of
> the commiters on this list for me to be supplying many smaller and less
> important patches all day long.
It is _always_ in the best interest (of you, and reviewers) to keep them very,
very short and sweet.
I spent a long time sending about four very large, and 'untrusted' patches
to the list. They were never applied. I stopped, looked at the very specific
things I wanted accomplished, and those patches were applied to httpd (1.3.12
at the time) very quickly.
The simple fact is, if it is obvious what you are trying to accomplish, and you
are only changing 'one thing', it is trivial for a committer to apply the patch.
If you change a dozen things (even name changes mixed with functional changes
mixed with retabulation or spacing adjustments) then it takes a great deal of
anyone's time to absorb it, and then determine what is right or wrong with it.
> The code-test-commit cycle is just way to long for that.
A five line change that is obvious will be committed within hours. Do that three
times a day and the patch that takes weeks to argue and be commited happens in a
matter of days.
> In any case, I will try to keep the patches short and
> sweet in the future.
Please do :)
> As for updating this patch, the only problem that I see is there was a
> variable left over that is no longer is use, and it's declaration should
> be removed. Do you still want me to revert all the 'thread' identifiers
> back to '(*new)'??
Of course! And offer up a seperate patch to change *only* the name of these
less-than-intuitive identifiers. Rbb is the last person who will ever try to
argue that his identifiers are the 'one true correct name' for anything :)
Change one bit at a time, to one end, and your patches can happen much more
quickly. While the rest of us are far from perfect in this, we really do try,
as committers, to break up formatting changes from functional changes, and our
functional changes into parsable units.
Bill
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Aaron Bannert <aa...@ebuilt.com>.
> > Readability, consistency, and even a slight performance boost.
> >
> > I think os2 had it right, so I changed the others to be the same.
> > Not really a name change at all.
>
> Still a name change, and at the very least this change has nothing to do with
> the fix that you are saying this patch is all about. This could and should have
> been submitted as a separate patch, because with this change, the patch is
> larger, and harder to review.
>
> If I remember back to my compiler class, I seem to believe that the value should
> have been moved into a register after the first reference, so I don't see a huge
> performance win. Of course, it has been a while since I took that class.
[warning: geek info]
There are of course no guarantees, and optimized register allocation
will only really apply within basic blocks, rarely across. When you
have function calls like apr_pcalloc(), all bets are off.
The only way to make sure on all compilers that it does [pretty much]
what you want it to would be to use a single-indirection (a pointer
to the struct), instead of double each time.
> > This change mirrors the same change to unix. Again, because I don't
> > have access to beos I can only do my best to get that code as close
> > as possible to what I think it should be, but in the end I'd need
> > someone who is familiar with that system to review my changes.
>
> Then you should have left the original code in tact. At the very least, this should be:
>
> if (wait_for_thread(thd->td, (void *)&thrstat) == B_NO_ERROR) {
> *retval = *thrstat;
> }
I'll leave this intact.
> > > > - *retval = (apr_status_t)thd->rv;
> > > > + if (retval != NULL)
> > > > + *retval = (apr_status_t)thd->rv;
> > >
> > > Again, we don't need this change.
> >
> > How is NULL an invalid return value from a thread? Here are the API
> > docs:
> >
> > /**
> > * block until the desired thread stops executing.
> > * @param retval The return value from the dead thread.
> > * @param thd The thread to join
> > * @deffunc apr_status_t apr_thread_join(apr_status_t *retval,
> > apr_thread_t *thd); */
> >
> > What if you don't want to pass it an apr_status_t, you just want to
> > join? My changes bring apr_thread_join() more inline with
> > pthread_join(). It's perfectly valid to give pthread_join() a NULL
> > and it won't bother trying to set the return value for you.
>
> You aren't programming to pthreads right now. You are programming for APR.
> They are different beasts, with different goals and priorities. In APR, return
> values mean something, and just ignoring them is not valid. If you don't want
> to return a value, then APR_SUCCESS should be returned.
Aha, you see: you can't just return APR_SUCCESS, since apr_thread_exit()
expects a *pointer* to an apr_success_t, and that means an integer's
going to have to be allocated from the heap. I think you'll agree that
in the least that is cumbersome, and more pratically it's a terribly
expensive operation to be performing for a simple status return.
So, should we revise the set of valid input parameters to apr_thread_exit()
and apr_thread_join(), or should we change the prototype to apr_thread_exit()?
> > This particular function is not in use *anywhere* in the httpd-2.0
> > or apr/apr-util code trees. AFAIK, flood is the only place where
> > this is being used. I would be very concerned about changing
> > functionality of existing code if that were not the case, but since
> > it is not I think there is MUCH room for interpretation.
>
> I disagree, and I designed this API. The difference is what you are passing
> around. In pthreads, you are passing in a (void **), which implies that the return
> type is unknown. In APR, it is an integer, and ALL threads exit with a return
> code. It doesn't matter where you know about the code being used. I happen to
> know of multiple closed source programs using APR, neither one of us knows
> everything about those programs so making decisions based on who is and who is
> not using a function doesn't work.
Like I said above, it's not an integer, it's a (apr_status_t*). However,
I see your point about this not being pthread.
I totally agree that in general we shouldn't be making decisions based on who
may or may not be using APR in their private code. My point still remains,
and your statement that "ALL threads exit with a return code" is false.
Read the source. httpd-2.0 returns NULL from a thread worker function
in 3 places that I count.
In an effort not to let this slow us down, I will remove these calls
from the original patch and repost.
> > > > @@ -178,8 +189,11 @@
> > > > apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t *thd) {
> > > > apr_status_t stat;
> > > > + apr_status_t *thrstat = NULL;
> > > >
> > > > - if ((stat = pthread_join(*thd->td,(void *)&retval)) == 0) {
> > > > + if ((stat = pthread_join(*thd->td,(void **)&thrstat)) == 0) {
> > > > + if (retval != NULL && thrstat != NULL)
> > > > + *retval = *thrstat;
> > >
> > > And again, this could have been fixed by just adding a * to the
> > > (void *), couldn't it?
> >
> > The cast just gets rid of the warning, but doesn't change the value
> > of the parameter. pthread_join() will set thrstat to point to the
> > value returned from apr_thread_exit(), but that is just a
> > pass-by-value parameter so it never makes it back out to the
> > caller. That is why it didn't work before.
>
> I must be missing something. retval is an (apr_status_t *), and thrstat
> is an (apr_status_t *), but one is going to work properly, and the other
> isn't? I'm sorry, what am I missing?
pthread_join() will set the return value of the thread to the
location pointed to by the second parameter.
1) We pass what amounts to an (int*) to apr_thread_join(). (Actually,
a copy is pushed onto the stack before the call).
2) We pass a pointer to this copied (int*) to pthread_join(), which
promptly sets that (int*) to point to the return value of the thread.
3) apr_thread_join() exits, and our copied (int*) falls out of scope,
never having actually affected the memory at the address in the (int*).
I will supply my original fix in a subsequent patch.
> > Your inspection is valid, but I'm not sure it is in the best
> > interest of the commiters on this list for me to be supplying many
> > smaller and less important patches all day long. The
> > code-test-commit cycle is just way to long for that. In any case, I
> > will try to keep the patches short and sweet in the future.
>
> Why do you think the code-test-commit cycle is too long? This patch has
> been out for under one week, which is far shorter than the months that
> others have to wait sometimes. The thing is, what is in the best interest of
> the committers, is for patches to be reviewed in detail. Posting five small patches
> that do one thing is a much better way to get patches reviewed. Shoot, one large
> patch that does one thing can be reviewed easily, but this is a large patch that
> does many things.
My objection is rescinded. I have seen the light of smaller patches. I
apoligise for putting too many things into one patch. I'm not quite used
to this whole OSS thing yet, afterall, one tends to see larger-scale
patches in the corporate settings I'm more used to. :)
> > As for updating this patch, the only problem that I see is there
> > was a variable left over that is no longer is use, and it's
> > declaration should be removed. Do you still want me to revert all
> > the 'thread' identifiers back to '(*new)'??
>
> See above, I still see more problems than you do. :-)
I'll resubmit shortly.
-aaron
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Ryan Bloom <rb...@covalent.net>.
> > > + apr_thread_t *thread;
> > > + apr_thread_param_t *thrparm;
> > >
> > > - (*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t)); - if ((*new) == NULL) {
> > > + thread = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t)); + (*new) = thread;
> >
> > Why did we add a new variable here? A BIG portion of this patch
> > could have been avoided, making it easier to review, if variable
> > name changes like this hadn't been included.
>
> Readability, consistency, and even a slight performance boost.
>
> I think os2 had it right, so I changed the others to be the same.
> Not really a name change at all.
Still a name change, and at the very least this change has nothing to do with
the fix that you are saying this patch is all about. This could and should have
been submitted as a separate patch, because with this change, the patch is
larger, and harder to review.
If I remember back to my compiler class, I seem to believe that the value should
have been moved into a register after the first reference, so I don't see a huge
performance win. Of course, it has been a while since I took that class.
> > > apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t *thd) {
> > > - if (wait_for_thread(thd->td,(void *)&retval) == B_NO_ERROR) {
> > > + apr_status_t *thrstat = NULL;
> > > +
> > > + if (wait_for_thread(thd->td,(void *)&thrstat) == B_NO_ERROR) {
> > > + if (retval != NULL && thrstat != NULL)
> > > + *retval = *thrstat;
> >
> > What is this change about? If the user passed in NULL for
> > retval, then we should just seg fault here. Why was this all
> > added?
>
> This change mirrors the same change to unix. Again, because I don't
> have access to beos I can only do my best to get that code as close
> as possible to what I think it should be, but in the end I'd need
> someone who is familiar with that system to review my changes.
Then you should have left the original code in tact. At the very least, this should be:
if (wait_for_thread(thd->td, (void *)&thrstat) == B_NO_ERROR) {
*retval = *thrstat;
}
> > > - *retval = (apr_status_t)thd->rv;
> > > + if (retval != NULL)
> > > + *retval = (apr_status_t)thd->rv;
> >
> > Again, we don't need this change.
>
> How is NULL an invalid return value from a thread? Here are the API
> docs:
>
> /**
> * block until the desired thread stops executing.
> * @param retval The return value from the dead thread.
> * @param thd The thread to join
> * @deffunc apr_status_t apr_thread_join(apr_status_t *retval,
> apr_thread_t *thd); */
>
> What if you don't want to pass it an apr_status_t, you just want to
> join? My changes bring apr_thread_join() more inline with
> pthread_join(). It's perfectly valid to give pthread_join() a NULL
> and it won't bother trying to set the return value for you.
You aren't programming to pthreads right now. You are programming for APR.
They are different beasts, with different goals and priorities. In APR, return
values mean something, and just ignoring them is not valid. If you don't want
to return a value, then APR_SUCCESS should be returned.
> This particular function is not in use *anywhere* in the httpd-2.0
> or apr/apr-util code trees. AFAIK, flood is the only place where
> this is being used. I would be very concerned about changing
> functionality of existing code if that were not the case, but since
> it is not I think there is MUCH room for interpretation.
I disagree, and I designed this API. The difference is what you are passing
around. In pthreads, you are passing in a (void **), which implies that the return
type is unknown. In APR, it is an integer, and ALL threads exit with a return
code. It doesn't matter where you know about the code being used. I happen to
know of multiple closed source programs using APR, neither one of us knows
everything about those programs so making decisions based on who is and who is
not using a function doesn't work.
> > > @@ -178,8 +189,11 @@
> > > apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t *thd) {
> > > apr_status_t stat;
> > > + apr_status_t *thrstat = NULL;
> > >
> > > - if ((stat = pthread_join(*thd->td,(void *)&retval)) == 0) {
> > > + if ((stat = pthread_join(*thd->td,(void **)&thrstat)) == 0) {
> > > + if (retval != NULL && thrstat != NULL)
> > > + *retval = *thrstat;
> >
> > And again, this could have been fixed by just adding a * to the
> > (void *), couldn't it?
>
> The cast just gets rid of the warning, but doesn't change the value
> of the parameter. pthread_join() will set thrstat to point to the
> value returned from apr_thread_exit(), but that is just a
> pass-by-value parameter so it never makes it back out to the
> caller. That is why it didn't work before.
I must be missing something. retval is an (apr_status_t *), and thrstat
is an (apr_status_t *), but one is going to work properly, and the other
isn't? I'm sorry, what am I missing?
> Your inspection is valid, but I'm not sure it is in the best
> interest of the commiters on this list for me to be supplying many
> smaller and less important patches all day long. The
> code-test-commit cycle is just way to long for that. In any case, I
> will try to keep the patches short and sweet in the future.
Why do you think the code-test-commit cycle is too long? This patch has
been out for under one week, which is far shorter than the months that
others have to wait sometimes. The thing is, what is in the best interest of
the committers, is for patches to be reviewed in detail. Posting five small patches
that do one thing is a much better way to get patches reviewed. Shoot, one large
patch that does one thing can be reviewed easily, but this is a large patch that
does many things.
> As for updating this patch, the only problem that I see is there
> was a variable left over that is no longer is use, and it's
> declaration should be removed. Do you still want me to revert all
> the 'thread' identifiers back to '(*new)'??
See above, I still see more problems than you do. :-)
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Aaron Bannert <aa...@ebuilt.com>.
On Fri, Jul 20, 2001 at 09:20:49PM -0700, Ryan Bloom wrote:
> On Friday 20 July 2001 10:30, Aaron Bannert wrote:
> > I submitted most of this patch last weekend. Since then I've fixed
> > a small bug in the apr_thread_exit() patch I made (I could really
> > use APR commit access for the little things like this :). I haven't
> > heard any objections (Ryan ;), so I assume these changes were
> > acceptable.
>
> Grrrrrr...... This patch does all sorts of stuff that it doesn't need to do. :-( The BeOS code was changed drastically, although most of the changes are more cosmetic than anything else, some are more serious. I have decided not to apply this patch because some of the changes are just not necessary, and others are just wrong given APR's general coding style. I have more comments in-line.
>
> > Index: srclib/apr/threadproc/beos/thread.c
> > ===================================================================
> > RCS file: /home/cvspublic/apr/threadproc/beos/thread.c,v
> > retrieving revision 1.22
> > diff -u -r1.22 thread.c
> > --- srclib/apr/threadproc/beos/thread.c 2001/06/14 18:52:05 1.22
> > +++ srclib/apr/threadproc/beos/thread.c 2001/07/20 17:11:33
> > @@ -94,13 +94,17 @@
> > {
> > int32 temp;
> > apr_status_t stat;
> > + struct thread_info_t *thr_info;
>
> What is this variable for? It's never used anywhere.
That was a mistake. I removed it in the unix tree, but missed in beos
(as I don't have beos).
>
> > + apr_thread_t *thread;
> > + apr_thread_param_t *thrparm;
> >
> > - (*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
> > - if ((*new) == NULL) {
> > + thread = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
> > + (*new) = thread;
>
> Why did we add a new variable here? A BIG portion of this patch could
> have been avoided, making it easier to review, if variable name changes
> like this hadn't been included.
Readability, consistency, and even a slight performance boost.
I think os2 had it right, so I changed the others to be the same. Not
really a name change at all.
> > apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t
> > *thd) {
> > - if (wait_for_thread(thd->td,(void *)&retval) == B_NO_ERROR) {
> > + apr_status_t *thrstat = NULL;
> > +
> > + if (wait_for_thread(thd->td,(void *)&thrstat) == B_NO_ERROR) {
> > + if (retval != NULL && thrstat != NULL)
> > + *retval = *thrstat;
>
> What is this change about? If the user passed in NULL for retval, then we
> should just seg fault here. Why was this all added?
This change mirrors the same change to unix. Again, because I don't have
access to beos I can only do my best to get that code as close as possible
to what I think it should be, but in the end I'd need someone who is
familiar with that system to review my changes.
> > - *retval = (apr_status_t)thd->rv;
> > + if (retval != NULL)
> > + *retval = (apr_status_t)thd->rv;
>
> Again, we don't need this change.
How is NULL an invalid return value from a thread? Here are the API docs:
/**
* block until the desired thread stops executing.
* @param retval The return value from the dead thread.
* @param thd The thread to join
* @deffunc apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t *thd);
*/
What if you don't want to pass it an apr_status_t, you just want to join?
My changes bring apr_thread_join() more inline with pthread_join().
It's perfectly valid to give pthread_join() a NULL and it won't bother
trying to set the return value for you.
This particular function is not in use *anywhere* in the httpd-2.0 or
apr/apr-util code trees. AFAIK, flood is the only place where this is
being used. I would be very concerned about changing functionality of
existing code if that were not the case, but since it is not I think
there is MUCH room for interpretation.
> > Index: srclib/apr/threadproc/unix/thread.c
> > ===================================================================
> > RCS file: /home/cvspublic/apr/threadproc/unix/thread.c,v
> > retrieving revision 1.39
> > diff -u -r1.39 thread.c
> > --- srclib/apr/threadproc/unix/thread.c 2001/06/14 18:52:09 1.39
> > +++ srclib/apr/threadproc/unix/thread.c 2001/07/20 17:11:33
> > @@ -122,32 +122,43 @@
> > {
> > apr_status_t stat;
> > pthread_attr_t *temp;
> > + apr_thread_t *thread;
> > + apr_thread_param_t *thrparm;
> >
> > - (*new) = (apr_thread_t *)apr_pcalloc(cont,
> > sizeof(apr_thread_t)); + thread = (apr_thread_t
> > *)apr_pcalloc(cont, sizeof(apr_thread_t)); + (*new) = thread;
> >
> > - if ((*new) == NULL) {
> > + if (thread == NULL) {
>
> Same as above. Changing variable names is not a good thing to
> do in the middle of a patch this size.
Same reason as before. Changing implementations to be consistent
improves readability. It also reduces the number of indirections.
> > @@ -178,8 +189,11 @@
> > apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t
> > *thd) {
> > apr_status_t stat;
> > + apr_status_t *thrstat = NULL;
> >
> > - if ((stat = pthread_join(*thd->td,(void *)&retval)) == 0) {
> > + if ((stat = pthread_join(*thd->td,(void **)&thrstat)) == 0) {
> > + if (retval != NULL && thrstat != NULL)
> > + *retval = *thrstat;
>
> And again, this could have been fixed by just adding a * to the (void *), couldn't it?
The cast just gets rid of the warning, but doesn't change the value of
the parameter. pthread_join() will set thrstat to point to the value
returned from apr_thread_exit(), but that is just a pass-by-value parameter
so it never makes it back out to the caller. That is why it didn't work
before.
> I have skipped the Windows section of the patch, but it is more of the same.
> Fix those comments, and I will review the patch again. MNSO is that the goal of
> patches should be to change as little code as possible. The more changes there are,
> the harder it is to review the patch. If a patch is generated, and there isn't a need for
> a line of code to have changed, the person who created the patch should go through
> and ask why that line was changed.
Your inspection is valid, but I'm not sure it is in the best interest of
the commiters on this list for me to be supplying many smaller and less
important patches all day long. The code-test-commit cycle is just way
to long for that. In any case, I will try to keep the patches short and
sweet in the future.
As for updating this patch, the only problem that I see is there was a
variable left over that is no longer is use, and it's declaration should
be removed. Do you still want me to revert all the 'thread' identifiers
back to '(*new)'??
-aaron
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Ryan Bloom <rb...@covalent.net>.
On Friday 20 July 2001 10:30, Aaron Bannert wrote:
> I submitted most of this patch last weekend. Since then I've fixed
> a small bug in the apr_thread_exit() patch I made (I could really
> use APR commit access for the little things like this :). I haven't
> heard any objections (Ryan ;), so I assume these changes were
> acceptable.
Grrrrrr...... This patch does all sorts of stuff that it doesn't need to do. :-( The BeOS code was changed drastically, although most of the changes are more cosmetic than anything else, some are more serious. I have decided not to apply this patch because some of the changes are just not necessary, and others are just wrong given APR's general coding style. I have more comments in-line.
> Index: srclib/apr/threadproc/beos/thread.c
> ===================================================================
> RCS file: /home/cvspublic/apr/threadproc/beos/thread.c,v
> retrieving revision 1.22
> diff -u -r1.22 thread.c
> --- srclib/apr/threadproc/beos/thread.c 2001/06/14 18:52:05 1.22
> +++ srclib/apr/threadproc/beos/thread.c 2001/07/20 17:11:33
> @@ -94,13 +94,17 @@
> {
> int32 temp;
> apr_status_t stat;
> + struct thread_info_t *thr_info;
What is this variable for? It's never used anywhere.
> + apr_thread_t *thread;
> + apr_thread_param_t *thrparm;
>
> - (*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
> - if ((*new) == NULL) {
> + thread = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
> + (*new) = thread;
Why did we add a new variable here? A BIG portion of this patch could
have been avoided, making it easier to review, if variable name changes
like this hadn't been included.
> apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t
> *thd) {
> - if (wait_for_thread(thd->td,(void *)&retval) == B_NO_ERROR) {
> + apr_status_t *thrstat = NULL;
> +
> + if (wait_for_thread(thd->td,(void *)&thrstat) == B_NO_ERROR) {
> + if (retval != NULL && thrstat != NULL)
> + *retval = *thrstat;
What is this change about? If the user passed in NULL for retval, then we
should just seg fault here. Why was this all added?
> - *retval = (apr_status_t)thd->rv;
> + if (retval != NULL)
> + *retval = (apr_status_t)thd->rv;
Again, we don't need this change.
> Index: srclib/apr/threadproc/unix/thread.c
> ===================================================================
> RCS file: /home/cvspublic/apr/threadproc/unix/thread.c,v
> retrieving revision 1.39
> diff -u -r1.39 thread.c
> --- srclib/apr/threadproc/unix/thread.c 2001/06/14 18:52:09 1.39
> +++ srclib/apr/threadproc/unix/thread.c 2001/07/20 17:11:33
> @@ -122,32 +122,43 @@
> {
> apr_status_t stat;
> pthread_attr_t *temp;
> + apr_thread_t *thread;
> + apr_thread_param_t *thrparm;
>
> - (*new) = (apr_thread_t *)apr_pcalloc(cont,
> sizeof(apr_thread_t)); + thread = (apr_thread_t
> *)apr_pcalloc(cont, sizeof(apr_thread_t)); + (*new) = thread;
>
> - if ((*new) == NULL) {
> + if (thread == NULL) {
Same as above. Changing variable names is not a good thing to
do in the middle of a patch this size.
> @@ -178,8 +189,11 @@
> apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t
> *thd) {
> apr_status_t stat;
> + apr_status_t *thrstat = NULL;
>
> - if ((stat = pthread_join(*thd->td,(void *)&retval)) == 0) {
> + if ((stat = pthread_join(*thd->td,(void **)&thrstat)) == 0) {
> + if (retval != NULL && thrstat != NULL)
> + *retval = *thrstat;
And again, this could have been fixed by just adding a * to the (void *), couldn't it?
I have skipped the Windows section of the patch, but it is more of the same.
Fix those comments, and I will review the patch again. MNSO is that the goal of
patches should be to change as little code as possible. The more changes there are,
the harder it is to review the patch. If a patch is generated, and there isn't a need for
a line of code to have changed, the person who created the patch should go through
and ask why that line was changed.
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Ryan Bloom <rb...@covalent.net>.
On Friday 20 July 2001 10:30, Aaron Bannert wrote:
> I submitted most of this patch last weekend. Since then I've fixed
> a small bug in the apr_thread_exit() patch I made (I could really
> use APR commit access for the little things like this :). I haven't
> heard any objections (Ryan ;), so I assume these changes were
> acceptable.
You submitted this a few days ago, and I haven't had time to review
it yet. I do not code full time anymore, so I review code as I have
time, and that has to compete with my actual coding. I have this
patch on my list of things to review this weekend.
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Aaron Bannert" <aa...@ebuilt.com>
Sent: Monday, July 23, 2001 12:24 PM
> On Mon, Jul 23, 2001 at 11:53:39AM -0500, William A. Rowe, Jr. wrote:
> > From: "Ryan Bloom" <rb...@covalent.net>
> > Sent: Saturday, July 21, 2001 11:55 AM
> >
> > I spoke with rbb who is traveling, and finally persuaded him that exactly two
> > thread arguments are reasonable, one is the single "apr's private and otherwise
> > useful data", a private apr_thread_info_t, which the user can get from accessor
> > calls
>
> (having a little trouble parsing this. for clarity, what will be the
> prototype of the worker_fn() -- (aka apr_thread_start_t)?)
typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_info_t *apr_info, void *usr_info);
> > ...and the second is the single "user's own useful data", the traditional
> > void* of a thread create call.
> >
> > This would restore some binary compatibility, since all the private details are
> > consistent within the library itself.
>
> This sounds similiar to the original patch I posted on this subject
> (the mega-patch that was rejected due to size :). The main difference
> between this and my original patch was that I was passing in three
> parameters: the apr private stuff, the
> application private stuff, and an extra param for the apr_pool_t struct.
> I was unaware of the accessor functions at that time.
Yup, that sounds like why we all diverged into three directions at once :)
> Whatever we decide on, would you all agree that it needs to solve these
> requirements:
>
> 1) the application's thread needs access to the child-pool that was created
> specifically for this thread.
Yes.
> 2) the application needs an obvious way to be able to call apr_thread_exit()
> (it currently requires the apr_thread_t).
I'd make your (apr_thread_info_t *) the argument to apr_thread_exit().
> Shall I cook up a final patch to get this done that does the following:
>
> - thread worker functions will now take 2 parameters, an apr_thread_t
> and the application-private opaque void* data.
No, the apr-private apr_thread_info_t *, and the app-private void*
> - updates to httpd that work with this.
Yes. A seperate patch :)
> - updates to test/testthread.c that illustrate this.
Yes. A seperate patch :)
> Open issues (please comment separately):
>
> - apr_thread_join() is still broken, but I am hesitant to provide the fixes
> for fear that they will conflict with previously posted patches.
Let's close this first
> - apr_thread_exit() takes an (apr_status_t*). Should that just be an
> apr_status_t instead? Does the application really need to allocate heap
> memory for a measily (int)?
As look as we are fixing the args to apr_thread_exit :) If the platform has
no idea about what to do with this, the apr implementation should have set aside
an int in the apr_thread_info_t.
Bill
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Aaron Bannert" <aa...@ebuilt.com>
Sent: Monday, July 23, 2001 5:44 PM
> Quick implementation question. I've got this working as described above
> under UNIX, but I'd like to do this correctly and I've run into a small
> issue.
>
> I'm not seeing a reason for having apr_thread_info_t in the first place,
> and I think we should just override apr_thread_t with the data we need.
> It's already private, and OS2 is already doing it this way. Are there
> any reasons I wouldn't just do it the same way? We would end up with
> something like this:
Hmmm... since it is private, I don't suppose there is a potential problem.
I'm fine, either way. Anyone seeing anything around the corner we've missed
with using the same apr_thread_t to both the creator and created threads?
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Aaron Bannert <aa...@ebuilt.com>.
On Mon, Jul 23, 2001 at 12:55:51PM -0500, William A. Rowe, Jr. wrote:
> From: "Aaron Bannert" <aa...@ebuilt.com>
> Sent: Monday, July 23, 2001 12:24 PM
>
>
> > On Mon, Jul 23, 2001 at 11:53:39AM -0500, William A. Rowe, Jr. wrote:
> > > From: "Ryan Bloom" <rb...@covalent.net>
> > > Sent: Saturday, July 21, 2001 11:55 AM
> > >
> > > I spoke with rbb who is traveling, and finally persuaded him that exactly two
> > > thread arguments are reasonable, one is the single "apr's private and otherwise
> > > useful data", a private apr_thread_info_t, which the user can get from accessor
> > > calls
> >
> > (having a little trouble parsing this. for clarity, what will be the
> > prototype of the worker_fn() -- (aka apr_thread_start_t)?)
>
> typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_info_t *apr_info, void *usr_info);
Quick implementation question. I've got this working as described above
under UNIX, but I'd like to do this correctly and I've run into a small
issue.
I'm not seeing a reason for having apr_thread_info_t in the first place,
and I think we should just override apr_thread_t with the data we need.
It's already private, and OS2 is already doing it this way. Are there
any reasons I wouldn't just do it the same way? We would end up with
something like this:
typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_t *apr_info, void *usr_info);
(either I'm missing something, or I don't know why I didn't offer this up
before as a compromise.)
-aaron
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Aaron Bannert" <aa...@ebuilt.com>
Sent: Monday, July 23, 2001 12:24 PM
> On Mon, Jul 23, 2001 at 11:53:39AM -0500, William A. Rowe, Jr. wrote:
> > From: "Ryan Bloom" <rb...@covalent.net>
> > Sent: Saturday, July 21, 2001 11:55 AM
> >
> > I spoke with rbb who is traveling, and finally persuaded him that exactly two
> > thread arguments are reasonable, one is the single "apr's private and otherwise
> > useful data", a private apr_thread_info_t, which the user can get from accessor
> > calls
>
> (having a little trouble parsing this. for clarity, what will be the
> prototype of the worker_fn() -- (aka apr_thread_start_t)?)
typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_info_t *apr_info, void *usr_info);
> > ...and the second is the single "user's own useful data", the traditional
> > void* of a thread create call.
> >
> > This would restore some binary compatibility, since all the private details are
> > consistent within the library itself.
>
> This sounds similiar to the original patch I posted on this subject
> (the mega-patch that was rejected due to size :). The main difference
> between this and my original patch was that I was passing in three
> parameters: the apr private stuff, the
> application private stuff, and an extra param for the apr_pool_t struct.
> I was unaware of the accessor functions at that time.
Yup, that sounds like why we all diverged into three directions at once :)
> Whatever we decide on, would you all agree that it needs to solve these
> requirements:
>
> 1) the application's thread needs access to the child-pool that was created
> specifically for this thread.
Yes.
> 2) the application needs an obvious way to be able to call apr_thread_exit()
> (it currently requires the apr_thread_t).
I'd make your (apr_thread_info_t *) the argument to apr_thread_exit().
> Shall I cook up a final patch to get this done that does the following:
>
> - thread worker functions will now take 2 parameters, an apr_thread_t
> and the application-private opaque void* data.
No, the apr-private apr_thread_info_t *, and the app-private void*
> - updates to httpd that work with this.
Yes. A seperate patch :)
> - updates to test/testthread.c that illustrate this.
Yes. A seperate patch :)
> Open issues (please comment separately):
>
> - apr_thread_join() is still broken, but I am hesitant to provide the fixes
> for fear that they will conflict with previously posted patches.
Let's close this first
> - apr_thread_exit() takes an (apr_status_t*). Should that just be an
> apr_status_t instead? Does the application really need to allocate heap
> memory for a measily (int)?
As look as we are fixing the args to apr_thread_exit :) If the platform has
no idea about what to do with this, the apr implementation should have set aside
an int in the apr_thread_info_t.
Bill
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Aaron Bannert <aa...@ebuilt.com>.
On Mon, Jul 23, 2001 at 11:53:39AM -0500, William A. Rowe, Jr. wrote:
> From: "Ryan Bloom" <rb...@covalent.net>
> Sent: Saturday, July 21, 2001 11:55 AM
>
> I spoke with rbb who is traveling, and finally persuaded him that exactly two
> thread arguments are reasonable, one is the single "apr's private and otherwise
> useful data", a private apr_thread_info_t, which the user can get from accessor
> calls
(having a little trouble parsing this. for clarity, what will be the
prototype of the worker_fn() -- (aka apr_thread_start_t)?)
> , and the second is the single "user's own useful data", the traditional
> void* of a thread create call.
>
> I really don't want this apr structure public, but I also don't want the user
> to be calling apr_thread_info_user_data_get() or some bogusness to accomplish
> what any thread coder does with a void* on any sane implementation.
>
> Can we accept two arguments to the user's thread fn? One is private and requires
> accessors to decypher, the other is private to the user's own application.
> It's a pretty clear boundry, what _is_ apr's, and what _isn't_.
>
> This would restore some binary compatibility, since all the private details are
> consistent within the library itself.
This sounds similiar to the original patch I posted on this subject
(the mega-patch that was rejected due to size :). The main difference
between this and my original patch was that I was passing in three
parameters: the apr private stuff, the
application private stuff, and an extra param for the apr_pool_t struct.
I was unaware of the accessor functions at that time.
Whatever we decide on, would you all agree that it needs to solve these
requirements:
1) the application's thread needs access to the child-pool that was created
specifically for this thread.
2) the application needs an obvious way to be able to call apr_thread_exit()
(it currently requires the apr_thread_t).
Shall I cook up a final patch to get this done that does the following:
- thread worker functions will now take 2 parameters, an apr_thread_t
and the application-private opaque void* data.
- updates to httpd that work with this.
- updates to test/testthread.c that illustrate this.
Open issues (please comment separately):
- apr_thread_join() is still broken, but I am hesitant to provide the fixes
for fear that they will conflict with previously posted patches.
- apr_thread_exit() takes an (apr_status_t*). Should that just be an
apr_status_t instead? Does the application really need to allocate heap
memory for a measily (int)?
-aaron
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Mon, Jul 23, 2001 at 11:53:39AM -0500, William A. Rowe, Jr. wrote:
> Can we accept two arguments to the user's thread fn? One is private and requires
> accessors to decypher, the other is private to the user's own application.
> It's a pretty clear boundry, what _is_ apr's, and what _isn't_.
+1 from the peanut gallery.
(I'm in the category of "I want this fixed, and I don't care much how it
gets fixed just so long as it does.") -- justin
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Ryan Bloom" <rb...@covalent.net>
Sent: Saturday, July 21, 2001 11:55 AM
I spoke with rbb who is traveling, and finally persuaded him that exactly two
thread arguments are reasonable, one is the single "apr's private and otherwise
useful data", a private apr_thread_info_t, which the user can get from accessor
calls, and the second is the single "user's own useful data", the traditional
void* of a thread create call.
I really don't want this apr structure public, but I also don't want the user
to be calling apr_thread_info_user_data_get() or some bogusness to accomplish
what any thread coder does with a void* on any sane implementation.
Can we accept two arguments to the user's thread fn? One is private and requires
accessors to decypher, the other is private to the user's own application.
It's a pretty clear boundry, what _is_ apr's, and what _isn't_.
This would restore some binary compatibility, since all the private details are
consistent within the library itself.
> > > > Not if we hide this detail, I believe. I don't know what that does
> > > > about providing access to the apr_thread_t in the child thread.
> > > > Perhaps we declare the user's func as accepting arguments (apr_thread_t
> > > > *me, void *mine).
> > >
> > > That was where Aaron started, and I asked him to change it to this model.
> > > This model is more extensible moving forward IMHO.
> >
> > It makes binary compatibility much more fragile, so I can't parse your
> > meaning for 'extensible'.
> >
> > Some public types have little to no reason to change, e.g. apr_finfo_t.
> >
> > Those that are more dynamic need to stay private.
>
> Well, this is more extensible than passing everything as a separate
> parameter, because it just requires a re-compile. If we go to separate
> parameters, then we require a code change. I don't believe that the
> requirements for this type will change anytime soon. Currently, we are
> passing the thread and the user's data, what else could EVERY thread need?
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Ryan Bloom <rb...@covalent.net>.
> > > Not if we hide this detail, I believe. I don't know what that does
> > > about providing access to the apr_thread_t in the child thread.
> > > Perhaps we declare the user's func as accepting arguments (apr_thread_t
> > > *me, void *mine).
> >
> > That was where Aaron started, and I asked him to change it to this model.
> > This model is more extensible moving forward IMHO.
>
> It makes binary compatibility much more fragile, so I can't parse your
> meaning for 'extensible'.
>
> Some public types have little to no reason to change, e.g. apr_finfo_t.
>
> Those that are more dynamic need to stay private.
Well, this is more extensible than passing everything as a separate
parameter, because it just requires a re-compile. If we go to separate
parameters, then we require a code change. I don't believe that the
requirements for this type will change anytime soon. Currently, we are
passing the thread and the user's data, what else could EVERY thread need?
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Ryan Bloom" <rb...@covalent.net>
Sent: Saturday, July 21, 2001 11:09 AM
> > > Changed the worker routine's signature to take a single parameter:
> > > apr_thread_param_t, which contains the opaque data and the apr_thread_t.
> >
> > Can we simply hide the details, and use our own _thread_main starting
> > point? Put the thread start function ptr in this structure as well, have
> > our _thread_main unwrap whatever is required (and it can create
> > thread-local data, if we ever implement that), finally calling out to the
> > user's *func?
> >
> > > httpd-2.0 will have to be updated after applying this patch to APR.
> >
> > Not if we hide this detail, I believe. I don't know what that does about
> > providing access to the apr_thread_t in the child thread. Perhaps we
> > declare the user's func as accepting arguments (apr_thread_t *me, void
> > *mine).
>
> That was where Aaron started, and I asked him to change it to this model.
> This model is more extensible moving forward IMHO.
It makes binary compatibility much more fragile, so I can't parse your meaning
for 'extensible'.
Some public types have little to no reason to change, e.g. apr_finfo_t.
Those that are more dynamic need to stay private.
[PATCH] update testthread.c
Posted by Aaron Bannert <aa...@ebuilt.com>.
On Sat, Jul 21, 2001 at 09:09:32AM -0700, Ryan Bloom wrote:
>
> > > Changed the worker routine's signature to take a single parameter:
> > > apr_thread_param_t, which contains the opaque data and the apr_thread_t.
> >
> > Can we simply hide the details, and use our own _thread_main starting
> > point? Put the thread start function ptr in this structure as well, have
> > our _thread_main unwrap whatever is required (and it can create
> > thread-local data, if we ever implement that), finally calling out to the
> > user's *func?
> >
> > > httpd-2.0 will have to be updated after applying this patch to APR.
> >
> > Not if we hide this detail, I believe. I don't know what that does about
> > providing access to the apr_thread_t in the child thread. Perhaps we
> > declare the user's func as accepting arguments (apr_thread_t *me, void
> > *mine).
>
> That was where Aaron started, and I asked him to change it to this model.
> This model is more extensible moving forward IMHO.
At this point for the sake of extensibility I agree.
I've attached an additional update for testthread.c to bring it up to date
with the new thread API. I've added a new test to check that the return
value from apr_thread_exit() is making it back to apr_thread_join()
properly -- it currently doesn't. Expect this test to fail until
apr_thread_join() is fixed (patch to come shortly).
-aaron
Index: srclib/apr/test/testthread.c
===================================================================
RCS file: /home/cvspublic/apr/test/testthread.c,v
retrieving revision 1.17
diff -u -r1.17 testthread.c
--- srclib/apr/test/testthread.c 2001/03/14 15:56:44 1.17
+++ srclib/apr/test/testthread.c 2001/07/23 06:17:02
@@ -58,6 +58,7 @@
#include "apr_general.h"
#include "errno.h"
#include <stdio.h>
+#include <stdlib.h>
#ifdef BEOS
#include <unistd.h>
#endif
@@ -72,57 +73,66 @@
}
#else /* !APR_HAS_THREADS */
-void * APR_THREAD_FUNC thread_func1(void *data);
-void * APR_THREAD_FUNC thread_func2(void *data);
-void * APR_THREAD_FUNC thread_func3(void *data);
-void * APR_THREAD_FUNC thread_func4(void *data);
+void * APR_THREAD_FUNC thread_func1(apr_thread_param_t *thrparm);
+void * APR_THREAD_FUNC thread_func2(apr_thread_param_t *thrparm);
+void * APR_THREAD_FUNC thread_func3(apr_thread_param_t *thrparm);
+void * APR_THREAD_FUNC thread_func4(apr_thread_param_t *thrparm);
apr_lock_t *thread_lock;
apr_pool_t *context;
int x = 0;
+apr_status_t exit_ret_val = 123; /* just some made up number to check on later */
-void * APR_THREAD_FUNC thread_func1(void *data)
+void * APR_THREAD_FUNC thread_func1(apr_thread_param_t *thrparm)
{
int i;
+ apr_thread_t *thread = thrparm->t;
for (i = 0; i < 10000; i++) {
apr_lock_acquire(thread_lock);
x++;
apr_lock_release(thread_lock);
}
+ apr_thread_exit(thread, &exit_ret_val);
return NULL;
}
-void * APR_THREAD_FUNC thread_func2(void *data)
+void * APR_THREAD_FUNC thread_func2(apr_thread_param_t *thrparm)
{
int i;
+ apr_thread_t *thread = thrparm->t;
for (i = 0; i < 10000; i++) {
apr_lock_acquire(thread_lock);
x++;
apr_lock_release(thread_lock);
}
+ apr_thread_exit(thread, &exit_ret_val);
return NULL;
}
-void * APR_THREAD_FUNC thread_func3(void *data)
+void * APR_THREAD_FUNC thread_func3(apr_thread_param_t *thrparm)
{
int i;
+ apr_thread_t *thread = thrparm->t;
for (i = 0; i < 10000; i++) {
apr_lock_acquire(thread_lock);
x++;
apr_lock_release(thread_lock);
}
+ apr_thread_exit(thread, &exit_ret_val);
return NULL;
}
-void * APR_THREAD_FUNC thread_func4(void *data)
+void * APR_THREAD_FUNC thread_func4(apr_thread_param_t *thrparm)
{
int i;
+ apr_thread_t *thread = thrparm->t;
for (i = 0; i < 10000; i++) {
apr_lock_acquire(thread_lock);
x++;
apr_lock_release(thread_lock);
}
+ apr_thread_exit(thread, &exit_ret_val);
return NULL;
}
@@ -172,7 +182,15 @@
apr_thread_join(&s2, t2);
apr_thread_join(&s3, t3);
apr_thread_join(&s4, t4);
- fprintf (stdout, "OK\n");
+ fprintf (stdout, "OK\n");
+
+ fprintf(stdout, "Checking thread's returned value.......");
+ if (s1 != exit_ret_val || s2 != exit_ret_val ||
+ s3 != exit_ret_val || s4 != exit_ret_val) {
+ fprintf(stderr, "Invalid return value (not expected value)\n");
+ exit(-1);
+ }
+ fprintf(stdout, "OK\n");
fprintf(stdout, "Checking if locks worked.......");
if (x != 40000) {
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Ryan Bloom <rb...@covalent.net>.
> > Changed the worker routine's signature to take a single parameter:
> > apr_thread_param_t, which contains the opaque data and the apr_thread_t.
>
> Can we simply hide the details, and use our own _thread_main starting
> point? Put the thread start function ptr in this structure as well, have
> our _thread_main unwrap whatever is required (and it can create
> thread-local data, if we ever implement that), finally calling out to the
> user's *func?
>
> > httpd-2.0 will have to be updated after applying this patch to APR.
>
> Not if we hide this detail, I believe. I don't know what that does about
> providing access to the apr_thread_t in the child thread. Perhaps we
> declare the user's func as accepting arguments (apr_thread_t *me, void
> *mine).
That was where Aaron started, and I asked him to change it to this model.
This model is more extensible moving forward IMHO.
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Aaron Bannert" <aa...@ebuilt.com>
Sent: Saturday, July 21, 2001 1:52 AM
> [Still trying to get the hang of this whole patch-submission/OSS project/
> mailing list thing :) -- this patch should be *much* smaller]
Much improved :)
> Update to APR thread API to explicitly pass the apr_thread_t to the
> worker function.
Good choice
> Changed the worker routine's signature to take a single parameter:
> apr_thread_param_t, which contains the opaque data and the apr_thread_t.
Can we simply hide the details, and use our own _thread_main starting point?
Put the thread start function ptr in this structure as well, have our
_thread_main unwrap whatever is required (and it can create thread-local
data, if we ever implement that), finally calling out to the user's *func?
> httpd-2.0 will have to be updated after applying this patch to APR.
Not if we hide this detail, I believe. I don't know what that does about
providing access to the apr_thread_t in the child thread. Perhaps we declare
the user's func as accepting arguments (apr_thread_t *me, void *mine).
Bill
> Index: srclib/apr/include/apr_thread_proc.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/apr_thread_proc.h,v
> retrieving revision 1.65
> diff -u -r1.65 apr_thread_proc.h
> --- srclib/apr/include/apr_thread_proc.h 2001/06/06 18:11:06 1.65
> +++ srclib/apr/include/apr_thread_proc.h 2001/07/21 06:43:58
> @@ -125,7 +125,13 @@
> typedef struct apr_other_child_rec_t apr_other_child_rec_t;
> #endif /* APR_HAS_OTHER_CHILD */
>
> -typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *);
> +struct apr_thread_param_t {
> + apr_thread_t *t;
> + void *data;
> +};
> +typedef struct apr_thread_param_t apr_thread_param_t;
> +
> +typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_param_t *);
My only objection is making this new struct 'public'. If we 'actually' invoked
our own static start_thread function
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Aaron Bannert <aa...@ebuilt.com>.
On Fri, Jul 20, 2001 at 11:52:36PM -0700, Aaron Bannert wrote:
> [Still trying to get the hang of this whole patch-submission/OSS project/
> mailing list thing :) -- this patch should be *much* smaller]
>
>
> Update to APR thread API to explicitly pass the apr_thread_t to the
> worker function.
>
> Changed the worker routine's signature to take a single parameter:
> apr_thread_param_t, which contains the opaque data and the apr_thread_t.
>
> httpd-2.0 will have to be updated after applying this patch to APR.
And here are the updates to httpd-2.0 mentioned above:
Changes the threaded MPM and the perchild MPM to work with the
new-and-improved APR thread API. All it really does it pull the
"data" parameter from the new apr_thread_param_t struct.
-aaron
Index: server/mpm/perchild/perchild.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/mpm/perchild/perchild.c,v
retrieving revision 1.71
diff -u -r1.71 perchild.c
--- server/mpm/perchild/perchild.c 2001/07/18 20:45:35 1.71
+++ server/mpm/perchild/perchild.c 2001/07/23 06:20:20
@@ -512,7 +512,7 @@
}
}
-static void *worker_thread(void *);
+static void *worker_thread(apr_thread_param_t *);
/* Starts a thread as long as we're below max_threads */
static int start_thread(void)
@@ -579,8 +579,9 @@
/* idle_thread_count should be incremented before starting a worker_thread */
-static void *worker_thread(void *arg)
+static void *worker_thread(apr_thread_param_t *param)
{
+ apr_thread_t *thread = param->t;
apr_socket_t *csd = NULL;
apr_pool_t *tpool; /* Pool for this thread */
apr_pool_t *ptrans; /* Pool for per-transaction stuff */
@@ -588,7 +589,7 @@
int srv;
int curr_pollfd, last_pollfd = 0;
int thread_just_started = 1;
- int thread_num = *((int *) arg);
+ int thread_num = *((int *) param->data);
long conn_id = child_num * HARD_THREAD_LIMIT + thread_num;
apr_pollfd_t *pollset;
int n;
Index: server/mpm/threaded/threaded.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/mpm/threaded/threaded.c,v
retrieving revision 1.46
diff -u -r1.46 threaded.c
--- server/mpm/threaded/threaded.c 2001/07/18 20:45:36 1.46
+++ server/mpm/threaded/threaded.c 2001/07/23 06:20:21
@@ -520,9 +520,10 @@
apr_lock_release(pipe_of_death_mutex);
}
-static void * worker_thread(void * dummy)
+static void * worker_thread(apr_thread_param_t *param)
{
- proc_info * ti = dummy;
+ apr_thread_t *thread = param->t;
+ proc_info * ti = param->data;
int process_slot = ti->pid;
int thread_slot = ti->tid;
apr_pool_t *tpool = ti->tpool;
@@ -671,9 +672,10 @@
return 0;
}
-static void *start_threads(void * dummy)
+static void *start_threads(apr_thread_param_t *param)
{
- thread_starter *ts = dummy;
+ apr_thread_t *thread = param->t;
+ thread_starter *ts = param->data;
apr_thread_t **threads = ts->threads;
apr_threadattr_t *thread_attr = ts->threadattr;
int child_num_arg = ts->child_num_arg;
Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Posted by Aaron Bannert <aa...@ebuilt.com>.
[Still trying to get the hang of this whole patch-submission/OSS project/
mailing list thing :) -- this patch should be *much* smaller]
Update to APR thread API to explicitly pass the apr_thread_t to the
worker function.
Changed the worker routine's signature to take a single parameter:
apr_thread_param_t, which contains the opaque data and the apr_thread_t.
httpd-2.0 will have to be updated after applying this patch to APR.
-aaron
Index: srclib/apr/include/apr_thread_proc.h
===================================================================
RCS file: /home/cvspublic/apr/include/apr_thread_proc.h,v
retrieving revision 1.65
diff -u -r1.65 apr_thread_proc.h
--- srclib/apr/include/apr_thread_proc.h 2001/06/06 18:11:06 1.65
+++ srclib/apr/include/apr_thread_proc.h 2001/07/21 06:43:58
@@ -125,7 +125,13 @@
typedef struct apr_other_child_rec_t apr_other_child_rec_t;
#endif /* APR_HAS_OTHER_CHILD */
-typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *);
+struct apr_thread_param_t {
+ apr_thread_t *t;
+ void *data;
+};
+typedef struct apr_thread_param_t apr_thread_param_t;
+
+typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_param_t *);
enum kill_conditions {
kill_never, /* process is never sent any signals */
Index: srclib/apr/threadproc/beos/thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/beos/thread.c,v
retrieving revision 1.22
diff -u -r1.22 thread.c
--- srclib/apr/threadproc/beos/thread.c 2001/06/14 18:52:05 1.22
+++ srclib/apr/threadproc/beos/thread.c 2001/07/21 06:43:58
@@ -94,6 +94,7 @@
{
int32 temp;
apr_status_t stat;
+ apr_thread_param_t *thrparm;
(*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
if ((*new) == NULL) {
@@ -113,7 +114,14 @@
return stat;
}
- (*new)->td = spawn_thread((thread_func)func, "apr thread", temp, data);
+ thrparm = (apr_thread_param_t *)apr_palloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+ thrparm->t = (*new);
+ thrparm->data = data;
+
+ (*new)->td = spawn_thread((thread_func)func, "apr thread", temp, thrparm);
/* Now we try to run it...*/
if (resume_thread((*new)->td) == B_NO_ERROR) {
return APR_SUCCESS;
Index: srclib/apr/threadproc/os2/thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/os2/thread.c,v
retrieving revision 1.22
diff -u -r1.22 thread.c
--- srclib/apr/threadproc/os2/thread.c 2001/06/16 01:27:15 1.22
+++ srclib/apr/threadproc/os2/thread.c 2001/07/21 06:43:59
@@ -94,8 +94,18 @@
static void apr_thread_begin(void *arg)
{
- apr_thread_t *thread = (apr_thread_t *)arg;
- thread->rv = thread->func(thread->data);
+ apr_thread_param_t *thrparm;
+ apr_thread_t *thread = (apr_thread_t *)arg;
+
+ thrparm = (apr_thread_param_t *)apr_palloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+
+ thrparm->t = thread;
+ thrparm->data = data;
+
+ thread->rv = thread->func(thrparm);
}
@@ -132,12 +142,23 @@
}
}
- if (thread->attr->attr & APR_THREADATTR_DETACHED)
+ if (thread->attr->attr & APR_THREADATTR_DETACHED) {
+ apr_thread_param_t *thrparm;
+
+ thrparm = (apr_thread_param_t *)apr_palloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+
+ thrparm->t = thread;
+ thrparm->data = data;
+
thread->tid = _beginthread((os2_thread_start_t)func, NULL,
- APR_THREAD_STACKSIZE, data);
- else
+ APR_THREAD_STACKSIZE, thrparm);
+ } else {
thread->tid = _beginthread(apr_thread_begin, NULL,
APR_THREAD_STACKSIZE, thread);
+ }
if (thread->tid < 0) {
return errno;
Index: srclib/apr/threadproc/unix/thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/unix/thread.c,v
retrieving revision 1.39
diff -u -r1.39 thread.c
--- srclib/apr/threadproc/unix/thread.c 2001/06/14 18:52:09 1.39
+++ srclib/apr/threadproc/unix/thread.c 2001/07/21 06:43:59
@@ -122,6 +122,7 @@
{
apr_status_t stat;
pthread_attr_t *temp;
+ apr_thread_param_t *thrparm;
(*new) = (apr_thread_t *)apr_pcalloc(cont, sizeof(apr_thread_t));
@@ -147,7 +148,15 @@
return stat;
}
- if ((stat = pthread_create((*new)->td, temp, func, data)) == 0) {
+ thrparm = (apr_thread_param_t *)apr_pcalloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+
+ thrparm->t = (*new);
+ thrparm->data = data;
+
+ if ((stat = pthread_create((*new)->td, temp, (void*)func, thrparm)) == 0) {
return APR_SUCCESS;
}
else {
Index: srclib/apr/threadproc/win32/thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/win32/thread.c,v
retrieving revision 1.32
diff -u -r1.32 thread.c
--- srclib/apr/threadproc/win32/thread.c 2001/07/06 14:20:03 1.32
+++ srclib/apr/threadproc/win32/thread.c 2001/07/21 06:43:59
@@ -97,6 +97,7 @@
apr_status_t stat;
unsigned temp;
int lasterror;
+ apr_thread_param_t *thrparm;
(*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
@@ -111,11 +112,19 @@
return stat;
}
+ thrparm = (apr_thread_param_t *)apr_palloc(cont, sizeof(apr_thread_param_t));
+ if (thrparm == NULL) {
+ return APR_ENOMEM;
+ }
+
+ thrparm->t = thread;
+ thrparm->data = data;
+
/* Use 0 for Thread Stack Size, because that will default the stack to the
* same size as the calling thread.
*/
if (((*new)->td = (HANDLE *)_beginthreadex(NULL, 0, (unsigned int (APR_THREAD_FUNC *)(void *))func,
- data, 0, &temp)) == 0) {
+ thrparm, 0, &temp)) == 0) {
lasterror = apr_get_os_error();
return APR_EEXIST;
/* MSVC++ doc doesn't mention any additional error info