Re: Some pending pathches for review/commit

[Max Khon]

Hi!

On Sun, Sep 19, 2004 at 05:47:48PM +0200, Mladen Turk wrote:

  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;

  +rv = APR_SUCCESS;

  }
 -/* Wait failed */
 -return apr_get_os_error();;
 +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)

/fjoe

Re: 1.0

[Max Khon]

Hi!

On Mon, Aug 09, 2004 at 08:57:09PM +0400, malc wrote:

 Perhaps im way off on this and please do correct me if i am wrong.
 
 Condition variables on Win32 are broken, if you are going to label
 APR with 1.0 mark and release it right now, without mentioning this
 fact in big red letters, this would essentially be equal to releasing
 a trojan horse - a free, attractive, portable thing with a stamp of
 greatness (Apache) in its name, but deadly.
 
 Not only code responsible for condvars under Win32 lacks any error
 checking whatsoever, there are also races, stuff which (to the best
 of my knowlege) results in upredictable behavior, so people using
 it will be bitten.. hard.

Threads are broken on win32 too (race condition in apr_thread_join and
apr_thread_exit).

/fjoe

Re: apr win32 bug [PATCH]

[Max Khon]

Hi!

On Sun, Aug 01, 2004 at 08:07:23PM -0500, William A. Rowe, Jr. wrote:

 Although I agree, with your patch in spirit, if apr_thread_join is never
 called, your patch -can- leak handles like a sieve :(
 
 Did we ever define that apr_thread_create() must be partnered with
 an apr_thread_join?

Yes.

From pthread_join manual page on Linux:

   When  a  joinable  thread  terminates,  its  memory  resources  (thread
   descriptor and stack) are not deallocated until another thread performs
   pthread_join on it. Therefore, pthread_join must  be  called  once  for
   each joinable thread created to avoid memory leaks.

In other words you MUST call apr_thread_join when underlying implementation
is pthreads.

 If not, it seems we need a clever way to mark
 the apr_thread_t HANDLE member as destroyed, and allow the
 apr_thread_join to simply return immediately.

This would be an overkill.

Please look here as well:
http://issues.apache.org/bugzilla/show_bug.cgi?id=28460 

 Bill
 
 At 04:52 PM 7/31/2004, Max Khon wrote:
 Hi!
 
 apr_thread_join for win32 is implemented incorrectly:
 thread handle is destroyed too early (in apr_thread_exit).
 If apr_thread_exit() is called before apr_thread_join() and
 new object is created (thread handle is reused) before
 calling apr_thread_join(), apr_thread_join() will possibly wait
 on invalid handle.
 
 Patch is attached.

apr win32 bug [PATCH]

[Max Khon]

Hi!

apr_thread_join for win32 is implemented incorrectly:
thread handle is destroyed too early (in apr_thread_exit).
If apr_thread_exit() is called before apr_thread_join() and
new object is created (thread handle is reused) before
calling apr_thread_join(), apr_thread_join() will possibly wait
on invalid handle.

Patch is attached.

/fjoe
Index: thread.c
===
RCS file: /home/cvspublic/apr/threadproc/win32/thread.c,v
retrieving revision 1.57
diff -u -p -r1.57 thread.c
--- thread.c10 Jun 2004 10:57:25 -  1.57
+++ thread.c31 Jul 2004 21:43:12 -
@@ -133,9 +133,6 @@ APR_DECLARE(apr_status_t) apr_thread_exi
 thd-exitval = retval;
 apr_pool_destroy(thd-pool);
 #ifndef _WIN32_WCE
-if (thd-td) {
-CloseHandle(thd-td);
-}
 _endthreadex(0);
 #else
 ExitThread(0);
@@ -149,6 +146,9 @@ APR_DECLARE(apr_status_t) apr_thread_joi
 apr_status_t rv;
 
 rv = WaitForSingleObject(thd-td, INFINITE);
+if (thd-td) {
+CloseHandle(thd-td);
+}
 if ( rv == WAIT_OBJECT_0 || rv == WAIT_ABANDONED) {
 *retval = thd-exitval;
 return APR_SUCCESS;