ChangeLogTag:Thu Sep 14 21:48:39 UTC 2006 Steve Huston <shuston@riverace.com>

2 files changed, 29 insertions, 19 deletions

diff --git a/ACE/ChangeLog b/ACE/ChangeLog
index 46db554c961..959cb0484ed 100644
--- a/ACE/ChangeLog
+++ b/ACE/ChangeLog
@@ -1,3 +1,27 @@
+Thu Sep 14 21:48:39 UTC 2006  Steve Huston  <shuston@riverace.com>
+
+	* ace/OS_NS_Thread.inl (thr_getspecific): Removed the ACE_Errno_Guard
+	  around ::TlsGetValue() in the ACE_HAS_WTHREADS case. The comments
+	  near the code said (paraphrasing) "...it was to protect against
+	  ACE_Log_Msg::instance() overwriting the error value before it had a
+	  chance to be logged; although ACE_ERROR et al already store the
+	  error value before calling ACE_Log_Msg::instance(), there may be a
+	  chance that other uses of ACE_Log_Msg don't protect this way."
+	  I have a report that having the errno guard in place is taking
+	  over 10% CPU during a customer system's run-time. This is way too
+	  much to have in a hot path, especially for a mis-placed guard.
+	  It seems a little random to be picking on ACE_OS::thr_getspecific()
+	  when there are many more OS calls in the ACE_Log_Msg::instance()
+	  call path.	
+	  If there are really cases outside of ACE_ERROR... et al, then the
+	  guard should be worked into ACE_Log_Msg::instance() method. However,
+	  rather than try that now, let's see if there are any real issues,
+	  since OS-level calls should be setting errno when errors are
+	  noticed, propagating from GetLastError() via
+	  ACE_OS::set_errno_to_last_error () as needed.
+	  Thanks to Kelly Hickel <kfh at mqsoftware dot com> for raising
+	  this point.
+
 Wed Sep 13 18:25:37 UTC 2006  Boris Kolpackov  <boris@codesynthesis.com>
 
         * ace/Bound_Ptr.h:
diff --git a/ACE/ace/OS_NS_Thread.inl b/ACE/ace/OS_NS_Thread.inl
index 675365afab2..349fffa2318 100644
--- a/ACE/ace/OS_NS_Thread.inl
+++ b/ACE/ace/OS_NS_Thread.inl
@@ -2809,26 +2809,12 @@ ACE_OS::thr_getspecific_native (ACE_OS_thread_key_t key, void **data)
     int result;
     ACE_OSCALL_RETURN (ACE_ADAPT_RETVAL (::thr_getspecific (key, data), result), int, -1);
 # elif defined (ACE_HAS_WTHREADS)
-
-  // The following handling of errno is designed like this due to
-  // ACE_Log_Msg::instance calling ACE_OS::thr_getspecific.
-  // Basically, it is ok for a system call to reset the error to zero.
-  // (It really shouldn't, though).  However, we have to remember to
-  // store errno *immediately* after an error is detected.  Calling
-  // ACE_ERROR_RETURN((..., errno)) did not work because errno was
-  // cleared before being passed to the thread-specific instance of
-  // ACE_Log_Msg.  The workaround for was to make it so
-  // thr_getspecific did not have the side effect of clearing errno.
-  // The correct fix is for ACE_ERROR_RETURN to store errno
-  //(actually ACE_OS::last_error) before getting the ACE_Log_Msg tss
-  // pointer, which is how it is implemented now.  However, other uses
-  // of ACE_Log_Msg may not work correctly, so we're keeping this as
-  // it is for now.
-
-  ACE_Errno_Guard error (errno);
   *data = ::TlsGetValue (key);
-  if (*data == 0 && (error = ::GetLastError ()) != NO_ERROR)
-    return -1;
+  if (*data == 0 && ::GetLastError () != NO_ERROR)
+    {
+      ACE_OS::set_errno_to_last_error ();
+      return -1;
+    }
   else
     return 0;
 # else /* ACE_HAS_PTHREADS etc.*/