List:Commits« Previous MessageNext Message »
From:kevin.lewis Date:January 9 2013 5:33pm
Subject:bzr push into mysql-trunk branch (kevin.lewis:5342 to 5343)
View as plain text  
 5343 kevin.lewis@stripped	2013-01-09
      Bug14762796 was first fixed in rb:1416, reviewed and approved by Jimmy.
      
      But that fix showed several test cases failing intermittently on PB2.
      CreateThread() does not immediately start the new thread so that in
      some small amount of calls to os_thread_create(), the CloseHandle()
      added by the previous patch actually closed the thread object before
      the thread itself could open a handle to it.  Then when the thread
      actually started, some of that thread info was lost.  This solution
      of closing the thread handle as soon as the thread was created was
      found in the MSDN documentation as a comment to the official docs.
      It never mentioned being timing dependent.
      
      I also noted that contrary to the MSDN documentation, not calling
      ThreadExit() from the thread itself before exiting in C++ code
      caused certain testcases to fail because of unreleased resources,
      at least some of the time on Windows 32-bit Vista.  I could never
      reproduce either problem on Windows 7-64.
      
      So I pushed a small patch to mysql-trunk to fix the failing
      testcases by adding a Sleep(0) before the CloseHandle() in
      os_thread_create() and to go back to ALWAYS doing ThreadExit() in
      os_thread_exit().  This seems to have cleaned up the failing testcases.
      
      But that 'Sleep(0)' solution is too indeterminate.  This patch
      introduces a global stl::map object to remember the Windows Handles
      returned from CreateThread() and then close them in os_thread_exit().
      This way they are not closed immediately while the thread is still
      getting started, which relys on thread switching and timing to work
      correctly.
      
      In addition, this patch makes sure that the Linux code always returns
      the new_thread_id in the thread_id parameter to os_thread_create().
      
      Approved by Sunny in rb:1788

    modified:
      storage/innobase/include/os0sync.h
      storage/innobase/os/os0sync.cc
      storage/innobase/os/os0thread.cc
 5342 Kristofer Pettersson	2013-01-09 [merge]
      automerge 5.6->trunk

=== modified file 'storage/innobase/include/os0sync.h'
--- a/storage/innobase/include/os0sync.h	revid:kristofer.pettersson@stripped
+++ b/storage/innobase/include/os0sync.h	revid:kevin.lewis@stripped
@@ -94,7 +94,8 @@ struct os_event {
 /** Operating system mutex handle */
 typedef struct os_mutex_t*	os_ib_mutex_t;
 
-/** Mutex protecting counts and the event and OS 'slow' mutex lists */
+/** Mutex protecting counts and the event and OS 'slow' mutex lists
+as well as the win_thread_map. */
 extern os_ib_mutex_t	os_sync_mutex;
 
 /** This is incremented by 1 in os_thread_create and decremented by 1 in

=== modified file 'storage/innobase/os/os0sync.cc'
--- a/storage/innobase/os/os0sync.cc	revid:kristofer.pettersson@stripped
+++ b/storage/innobase/os/os0sync.cc	revid:kevin.lewis@stripped
@@ -51,7 +51,8 @@ struct os_mutex_t{
 				/* list of all 'slow' OS mutexes created */
 };
 
-/** Mutex protecting counts and the lists of OS mutexes and events */
+/** Mutex protecting counts and the event and OS 'slow' mutex lists
+as well as the win_thread_map. */
 UNIV_INTERN os_ib_mutex_t	os_sync_mutex;
 /** TRUE if os_sync_mutex has been initialized */
 static ibool		os_sync_mutex_inited	= FALSE;

=== modified file 'storage/innobase/os/os0thread.cc'
--- a/storage/innobase/os/os0thread.cc	revid:kristofer.pettersson@stripped
+++ b/storage/innobase/os/os0thread.cc	revid:kevin.lewis@stripped
@@ -36,6 +36,12 @@ Created 9/8/1995 Heikki Tuuri
 #include "srv0srv.h"
 #include "os0sync.h"
 
+#ifdef __WIN__
+/** This STL map remembers the initial handle returned by CreateThread
+so that it can be closed when the thread exits. */
+static std::map<DWORD, HANDLE>	win_thread_map;
+#endif /* __WIN__ */
+
 /***************************************************************//**
 Compares two thread ids for equality.
 @return	TRUE if equal */
@@ -116,34 +122,22 @@ os_thread_create_func(
 	os_thread_id_t*		thread_id)	/*!< out: id of the created
 						thread, or NULL */
 {
+	os_thread_id_t	new_thread_id;
+
 #ifdef __WIN__
 	HANDLE		handle;
-	DWORD		win_thread_id;
 
 	os_mutex_enter(os_sync_mutex);
-	os_thread_count++;
-	os_mutex_exit(os_sync_mutex);
 
 	handle = CreateThread(NULL,	/* no security attributes */
 			      0,	/* default size stack */
 			      func,
 			      arg,
 			      0,	/* thread runs immediately */
-			      &win_thread_id);
+			      &new_thread_id);
 
-	if (handle) {
-		if (thread_id) {
-			*thread_id = win_thread_id;
-		}
-
-		/* Innodb does not use the handle outside this thread.
-		It only uses the thread_id.  So close the handle now.
-		The thread object is held open by the thread until it
-		exits. */
-		Sleep(0);
-		CloseHandle(handle);
-
-	} else {
+	if (!handle) {
+		os_mutex_exit(os_sync_mutex);
 		/* If we cannot start a new thread, life has no meaning. */
 		fprintf(stderr,
 			"InnoDB: Error: CreateThread returned %d\n",
@@ -152,10 +146,19 @@ os_thread_create_func(
 		exit(1);
 	}
 
+	std::pair<map<DWORD, HANDLE>::iterator,bool> ret;
+	ret = win_thread_map.insert(
+		std::pair<DWORD, HANDLE>(new_thread_id, handle));
+	ut_ad((*ret.first).first == new_thread_id);
+	ut_ad((*ret.first).second == handle);
+	ut_a(ret.second == true);	/* true means thread_id was new */
+	os_thread_count++;
+
+	os_mutex_exit(os_sync_mutex);
+
 #else /* __WIN__ else */
 
 	int		ret;
-	os_thread_id_t	pthread_id;
 	pthread_attr_t	attr;
 
 #ifndef UNIV_HPUX10
@@ -184,9 +187,9 @@ os_thread_create_func(
 	os_mutex_exit(os_sync_mutex);
 
 #ifdef UNIV_HPUX10
-	ret = pthread_create(&pthread_id, pthread_attr_default, func, arg);
+	ret = pthread_create(&new_thread_id, pthread_attr_default, func, arg);
 #else
-	ret = pthread_create(&pthread_id, &attr, func, arg);
+	ret = pthread_create(&new_thread_id, &attr, func, arg);
 #endif
 	if (ret) {
 		fprintf(stderr,
@@ -198,7 +201,13 @@ os_thread_create_func(
 #ifndef UNIV_HPUX10
 	pthread_attr_destroy(&attr);
 #endif
+
 #endif /* not __WIN__ */
+
+	/* Return the thread_id if the caller requests it. */
+	if (thread_id != NULL) {
+		*thread_id = new_thread_id;
+	}
 }
 
 /*****************************************************************//**
@@ -221,11 +230,19 @@ os_thread_exit(
 
 	os_mutex_enter(os_sync_mutex);
 	os_thread_count--;
-	os_mutex_exit(os_sync_mutex);
 
 #ifdef __WIN__
+	DWORD win_thread_id = GetCurrentThreadId();
+	HANDLE handle = win_thread_map[win_thread_id];
+	CloseHandle(handle);
+	int ret = win_thread_map.erase(win_thread_id);
+	ut_a(ret == 1);
+
+	os_mutex_exit(os_sync_mutex);
+
 	ExitThread((DWORD) exit_value);
 #else
+	os_mutex_exit(os_sync_mutex);
 	pthread_detach(pthread_self());
 	pthread_exit(exit_value);
 #endif

No bundle (reason: useless for push emails).
Thread
bzr push into mysql-trunk branch (kevin.lewis:5342 to 5343) kevin.lewis14 Feb