You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Mladen Turk <mt...@apache.org> on 2004/08/27 20:45:44 UTC
[PATCH] WIN32 - fix apr_thread_join (PR: 28460) - v.2
Hi,
Second try :)
To be consistent, the patch now follows the unix behavior.
a) If the thread is detached don't allow thread_join
but rather return APR_DETACHED.
b) If not detached, expect that thread_join will be
called.
c) If thread_exit was never called before thread_join
do not set the retval but rather return APR_INCOMPLETE.
cvs diff -u thread.c (in directory
C:\WRKTOOLS\WINCVS\DATA\apache\apr\threadproc\win32\)
Index: thread.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/win32/thread.c,v
retrieving revision 1.57
diff -u -r1.57 thread.c
--- thread.c 10 Jun 2004 10:57:25 -0000 1.57
+++ thread.c 27 Aug 2004 18:34:44 -0000
@@ -85,6 +85,7 @@
{
apr_status_t stat;
unsigned temp;
+ HANDLE handle;
(*new) = (apr_thread_t *)apr_palloc(pool, sizeof(apr_thread_t));
@@ -95,7 +96,7 @@
(*new)->pool = pool;
(*new)->data = data;
(*new)->func = func;
-
+ (*new)->td = NULL;
stat = apr_pool_create(&(*new)->pool, pool);
if (stat != APR_SUCCESS) {
return stat;
@@ -105,14 +106,14 @@
* same size as the calling thread.
*/
#ifndef _WIN32_WCE
- if (((*new)->td = (HANDLE)_beginthreadex(NULL,
+ if ((handle = (HANDLE)_beginthreadex(NULL,
attr && attr->stacksize > 0 ? attr->stacksize : 0,
(unsigned int (APR_THREAD_FUNC *)(void
*))dummy_worker,
(*new), 0, &temp)) == 0) {
return APR_FROM_OS_ERROR(_doserrno);
}
#else
- if (((*new)->td = CreateThread(NULL,
+ if ((handle = CreateThread(NULL,
attr && attr->stacksize > 0 ? attr->stacksize : 0,
(unsigned int (APR_THREAD_FUNC *)(void
*))dummy_worker,
(*new), 0, &temp)) == 0) {
@@ -120,9 +121,10 @@
}
#endif
if (attr && attr->detach) {
- CloseHandle((*new)->td);
- (*new)->td = NULL;
+ CloseHandle(handle);
}
+ else
+ (*new)->td = handle;
return APR_SUCCESS;
}
@@ -132,10 +134,8 @@
{
thd->exitval = retval;
apr_pool_destroy(thd->pool);
+ thd->pool = NULL;
#ifndef _WIN32_WCE
- if (thd->td) {
- CloseHandle(thd->td);
- }
_endthreadex(0);
#else
ExitThread(0);
@@ -146,15 +146,26 @@
APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval,
apr_thread_t *thd)
{
- apr_status_t rv;
-
+ apr_status_t rv = APR_SUCCESS;
+
+ if (!thd->td) {
+ /* Can not join on detached threads */
+ return APR_DETACH;
+ }
rv = WaitForSingleObject(thd->td, INFINITE);
if ( rv == WAIT_OBJECT_0 || rv == WAIT_ABANDONED) {
- *retval = thd->exitval;
- return APR_SUCCESS;
- }
- /* Wait failed */
- return apr_get_os_error();;
+ /* If the thread_exit has been called */
+ if (!thd->pool)
+ *retval = thd->exitval;
+ else
+ rv = APR_INCOMPLETE;
+ }
+ else
+ rv = apr_get_os_error();
+ CloseHandle(thd->td);
+ thd->td = NULL;
+
+ return rv;
}
APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
Regards,
MT.
Re: [PATCH] WIN32 - fix apr_thread_join (PR: 28460) - v.2
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:24 PM 8/27/2004, Mladen Turk wrote:
>Cliff Woolley wrote:
>
>>On Fri, 27 Aug 2004, Mladen Turk wrote:
>>
>>>c) If thread_exit was never called before thread_join
>>> do not set the retval but rather return APR_INCOMPLETE.
>>
>>That's really what the unix impl does?
>
>Probably not :)
>Didn't test what happens if you call the thread_join
>without ever calling thread_exit.
>
>> APR_INCOMPLETE was supposed to
>>mean that partial results were returned but just not quite everything the
>>caller asked for (as in the case of stat()ing a file but only getting back
>>some of the information the user wanted rather than all). Surely this is
>>an error condition, right? In that case, one of the APR_E* codes would be
>>more appropriate. Or maybe I'm not correctly understanding the situation
>>you're describing...
>
>APR_INCOMLETE says :
>The operation was incomplete although some processing
>was performed and the results are partially valid
>
>This rv can only happen if the code has been wrongly written,
>meaning that the user didn't call the thread_exit inside the
>thread func, but sill wishes to retrieve the thread exit
>value using thread_join.
>
>Because the thread handle is closed the operation is
>incomplete, meaning: OK, I can close the thread, but
>can not give you the thread's return value, cause you
>never set that.
>
>Anyhow, any error value will do as long as it makes sense :)
It doesn't because you didn't get a partial-results, you got no
results (it wasn't done.) I like the idea of researching what
Unix reports and matching it.
Bill
Re: [PATCH] WIN32 - fix apr_thread_join (PR: 28460) - v.2
Posted by Mladen Turk <mt...@apache.org>.
Cliff Woolley wrote:
> On Fri, 27 Aug 2004, Mladen Turk wrote:
>
>
>>c) If thread_exit was never called before thread_join
>> do not set the retval but rather return APR_INCOMPLETE.
>
>
> That's really what the unix impl does?
Probably not :)
Didn't test what happens if you call the thread_join
without ever calling thread_exit.
> APR_INCOMPLETE was supposed to
> mean that partial results were returned but just not quite everything the
> caller asked for (as in the case of stat()ing a file but only getting back
> some of the information the user wanted rather than all). Surely this is
> an error condition, right? In that case, one of the APR_E* codes would be
> more appropriate. Or maybe I'm not correctly understanding the situation
> you're describing...
>
APR_INCOMLETE says :
The operation was incomplete although some processing
was performed and the results are partially valid
This rv can only happen if the code has been wrongly written,
meaning that the user didn't call the thread_exit inside the
thread func, but sill wishes to retrieve the thread exit
value using thread_join.
Because the thread handle is closed the operation is
incomplete, meaning: OK, I can close the thread, but
can not give you the thread's return value, cause you
never set that.
Anyhow, any error value will do as long as it makes sense :)
Regards,
MT.
Re: [PATCH] WIN32 - fix apr_thread_join (PR: 28460) - v.2
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 27 Aug 2004, Mladen Turk wrote:
> c) If thread_exit was never called before thread_join
> do not set the retval but rather return APR_INCOMPLETE.
That's really what the unix impl does? APR_INCOMPLETE was supposed to
mean that partial results were returned but just not quite everything the
caller asked for (as in the case of stat()ing a file but only getting back
some of the information the user wanted rather than all). Surely this is
an error condition, right? In that case, one of the APR_E* codes would be
more appropriate. Or maybe I'm not correctly understanding the situation
you're describing...
--Cliff
PS, following are a couple of style nits, just fyi.
> unsigned temp;
> + HANDLE handle;
Please be careful not to introduce tabs into the code (that applies to apr
as well as httpd by the way). I usually just set my MSVC editor and vim
both to always expand tabs (I think MSVC calls it "insert spaces").
> #ifndef _WIN32_WCE
> - if (((*new)->td = (HANDLE)_beginthreadex(NULL,
> + if ((handle = (HANDLE)_beginthreadex(NULL,
> attr && attr->stacksize > 0 ? attr->stacksize : 0,
> (unsigned int (APR_THREAD_FUNC *)(void *))dummy_worker,
> (*new), 0, &temp)) == 0) {
Usually when we wrap arguments, it goes like this:
calltosomereallylongfunctionnameblahblahblah(argument1,
argument2,
argument3);
In this case, I think the original was trying to compensate for the fact
that the arguments themselves were also very long, so it used the
alternate method of indenting four spaces from the = sign on the first
row. Since the = sign has now shifted to the left, the arguments should
shift to the left by the same amount.
> + else
> + (*new)->td = handle;
Go ahead and put the {}'s around the else case here... we almost always
use them even if the block is only one line, just for defensive
programming purposes (aka, future-proofness).