Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (herse.apache.org: domain of bojan@rexursive.com designates
 203.171.74.242 as permitted sender)
Subject: Re: Time to revisit 1.2.9 flavors
From: Bojan Smojver <bojan@rexursive.com>
To: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
Cc: dev@apr.apache.org
In-Reply-To: <46446C43.3090100@rowe-clan.net>
References: <46321AEE.4080300@rowe-clan.net>
	 <1177714964.3178.1.camel@shrek.rexursive.com>
	 <1178862821.29406.71.camel@shrek.rexursive.com>
	 <46446C43.3090100@rowe-clan.net>
Content-Type: multipart/mixed; boundary="=-mvhXAOt0oSp16AEB1x59"
Date: Sat, 12 May 2007 14:30:31 +1000
Message-Id: <1178944231.29406.96.camel@shrek.rexursive.com>
Mime-Version: 1.0


--=-mvhXAOt0oSp16AEB1x59
Content-Type: text/plain
Content-Transfer-Encoding: 7bit

On Fri, 2007-05-11 at 08:14 -0500, William A. Rowe, Jr. wrote:

> Trouble with the submitted patch is that it isn't thread safe, I see all
> sorts of subtle races in this code :(

Yes, I was afraid someone may notice :-(

I'm attaching a patch that features a mutex to make things safer.
Hopefully a bit better. Also, I dropped OS2 and Win32 stuff - no need.

> But maybe I'm mis-reading things - does this only get toggled within the
> child process after fork()?

I think so.

-- 
Bojan

--=-mvhXAOt0oSp16AEB1x59
Content-Disposition: attachment; filename=apr-cleanup_for_exec-mutex.patch
Content-Type: text/x-patch; name=apr-cleanup_for_exec-mutex.patch;
 charset=utf-8
Content-Transfer-Encoding: 7bit

Index: memory/unix/apr_pools.c
===================================================================
--- memory/unix/apr_pools.c	(revision 537319)
+++ memory/unix/apr_pools.c	(working copy)
@@ -515,6 +515,11 @@
 static apr_file_t *file_stderr = NULL;
 #endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE_ALL) */
 
+static int cleanup_for_exec = 0;
+#if APR_HAS_THREADS
+static apr_thread_mutex_t *cleanup_for_exec_mutex = NULL;
+#endif
+
 /*
  * Local functions
  */
@@ -577,6 +582,14 @@
 
     apr_allocator_owner_set(global_allocator, global_pool);
 
+#if APR_HAS_THREADS
+    if ((rv = apr_thread_mutex_create(&cleanup_for_exec_mutex,
+                                      APR_THREAD_MUTEX_DEFAULT,
+                                      global_pool)) != APR_SUCCESS) {
+        return rv;
+    }
+#endif /* APR_HAS_THREADS */
+
     return APR_SUCCESS;
 }
 
@@ -2100,6 +2113,11 @@
         cleanup_pool_for_exec(p);
 }
 
+int apr_cleanup_is_for_exec()
+{
+    return cleanup_for_exec;
+}
+
 APR_DECLARE(void) apr_pool_cleanup_for_exec(void)
 {
 #if !defined(WIN32) && !defined(OS2)
@@ -2110,9 +2128,21 @@
      * be automajically closed. The only problem is with
      * file handles that are open, but there isn't much
      * I can do about that (except if the child decides
-     * to go out and close them
+     * to go out and close them).
      */
+
+#if APR_HAS_THREADS
+    apr_thread_mutex_lock(cleanup_for_exec_mutex);
+#endif
+
+    cleanup_for_exec = 1;
     cleanup_pool_for_exec(global_pool);
+    cleanup_for_exec = 0;
+
+#if APR_HAS_THREADS
+    apr_thread_mutex_unlock(cleanup_for_exec_mutex);
+#endif
+
 #endif /* !defined(WIN32) && !defined(OS2) */
 }
 
Index: include/arch/apr_private_common.h
===================================================================
--- include/arch/apr_private_common.h	(revision 537319)
+++ include/arch/apr_private_common.h	(working copy)
@@ -39,4 +39,7 @@
 #define APR_UINT32_TRUNC_CAST apr_uint32_t
 #define APR_UINT32_MAX        0xFFFFFFFFUL
 
+/* cleanup_for_exec */
+int apr_cleanup_is_for_exec();
+
 #endif  /*APR_PRIVATE_COMMON_H*/
Index: file_io/unix/readwrite.c
===================================================================
--- file_io/unix/readwrite.c	(revision 537319)
+++ file_io/unix/readwrite.c	(working copy)
@@ -319,6 +319,10 @@
 
 APR_DECLARE(apr_status_t) apr_file_flush(apr_file_t *thefile)
 {
+    if (apr_cleanup_is_for_exec()) {
+        return APR_SUCCESS;
+    }
+
     if (thefile->buffered) {
         if (thefile->direction == 1 && thefile->bufpos) {
             apr_ssize_t written;

--=-mvhXAOt0oSp16AEB1x59--