Re: gthread-win32.c

[Steven Brooks <umbrook0 cs umanitoba ca>]

> Does someone with more thread understanding than me have the time 
> to take a look through the source and point out open issues ?

Without actually running the code, here are my comments.


> --=====================_989764663==_
> Content-Type: text/plain; charset="us-ascii"
> Content-Disposition: attachment; filename="gthread-win32.c"
>
> /* GLIB - Library of useful routines for C programming
>  * Copyright (C) 1995-1997  Peter Mattis, Spencer Kimball and Josh MacDonald
>  *
>  * gthread.c: solaris thread system implementation
>  * Copyright 1998 Sebastian Wilhelmi; University of Karlsruhe
>  * Copyright 2000 Hans Breuer
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
>  * License as published by the Free Software Foundation; either
>  * version 2 of the License, or (at your option) any later version.
>  *
>  * This library is distributed in the hope that it will be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>  * Lesser General Public License for more details.
>  *
>  * You should have received a copy of the GNU Lesser General Public
>  * License along with this library; if not, write to the
>  * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>  * Boston, MA 02111-1307, USA.
>  */
>
> /*
>  * Modified by the GLib Team and others 1997-2000.  See the AUTHORS
>  * file for a list of people on the GLib Team.  See the ChangeLog
>  * files for a list of changes.  These files are distributed with
>  * GLib at ftp://ftp.gtk.org/pub/gtk/. 
>  */
>
> /* 
>  * MT safe
>  */
>
> #ifdef HAVE_CONFIG_H
> #include <config.h>
> #endif
>
> #include <glib.h>
> #define STRICT
> #include <windows.h>
>
> #define win32_print_error( name, num )				\
>   g_error( "file %s: line %d (%s): error %s during %s",			\
>            __FILE__, __LINE__, G_GNUC_PRETTY_FUNCTION,			\
>            g_strerror((num)), #name )
>
> #define win32_check_for_error( what ) G_STMT_START{			\
>   int error = (what);							\
>   if( error ) { win32_print_error( what, error ); }			\
>   }G_STMT_END
>
> gulong g_thread_min_stack_size = 0;
>
> #define G_MUTEX_SIZE (sizeof (HANDLE))
>
> #define PRIORITY_LOW_VALUE 0
> #define PRIORITY_URGENT_VALUE 127
>
> static GMutex *
> g_mutex_new_win32_impl (void)
> {
>   HANDLE *result = g_new (HANDLE, 1);
>   *result = CreateMutex (NULL, /* pointer to security attributes */
>                          TRUE, /* ??? flag for initial ownership */ 
>                          NULL); /* pointer to mutex-object name */
>   win32_check_for_error (NULL != *result);
>   return (GMutex *) result;
> }

The second parameter to CreateMutex (marked ???) indicates whether the
mutex is created in a locked or not-locked mode.  When set to TRUE,
the mutex is created locked by the current thread (i.e. owned by the
current thread).  When FALSE, the mutex is not locked by any threads
(i.e. not owned).

I'm not sure what the GLib standard is.  Looking at the API docs,
it doesn't say.  My suspicion is that mutexes should be initialised
unlocked.


> static void
> g_mutex_free_win32_impl (GMutex * mutex)
> {
>   win32_check_for_error (
>     CloseHandle (*(HANDLE*)mutex)
>     );
>   free (mutex);
> }
>
> /* NOTE: the functions g_mutex_lock and g_mutex_unlock may not use
>    functions from gmem.c and gmessages.c; */
>
> static void
> g_mutex_lock_win32_impl (GMutex *mutex)
> {
>   win32_check_for_error (
>     WAIT_FAILED != WaitForSingleObject (*(HANDLE*)mutex, INFINITE)
>     );
> }
>
> static void
> g_mutex_unlock_win32_impl (GMutex *mutex)
> {
>   win32_check_for_error (
>     ReleaseMutex (*(HANDLE*)mutex)
>     );
> }
>
> static gboolean
> g_mutex_trylock_win32_impl (GMutex * mutex)
> {
>   DWORD result;
>   result = WaitForSingleObject (*(HANDLE*)mutex, 0);
>   if (result == WAIT_OBJECT_0)
>     return TRUE;
>   win32_check_for_error (WAIT_TIMEOUT != result);
>   return FALSE;
> }
>
> static GCond *
> g_cond_new_win32_impl (void)
> {
>   HANDLE *result = g_new (HANDLE, 1);
>   *result = CreateEvent (NULL,  /* default security, can't inherit */
>                          FALSE, /* auto-reset */
>                          FALSE, /* initially nonsignaled */
>                          NULL); /* no name */
>
>   win32_check_for_error (NULL != *result);
>   return (GCond *) result;
> }
>
> static void
> g_cond_signal_win32_impl (GCond * cond)
> {
>   win32_check_for_error (SetEvent (*(HANDLE*)cond));
> }
>
> static void
> g_cond_broadcast_win32_impl (GCond * cond)
> {
>   /* XXX : signal all waiters ? */
> }

No way (that I know of) to do this in windows.  At least, not through
the Win32 API.  An event created to auto-reset will always signal
exactly one thread. An event created to manual-reset will always signal
all threads.

One possibility would be to make the event an object instead of just
a Window's event.  It could keep track of how many threads are waiting
on the event and send that many pulses.  Although there would be race
conditions involved.

A similar solution would be that each thread, upon waking, checks if the
event is being broadcast (through some sort of flag), and if it is, and
there are more threads waiting, it sends a PulseEvent and then gives up
its timeslice.  That should help ensure that all threads wake up before
any of the threads actually run -- but there would be no real guarantee
without some kind of barrier.

However, an ``almost perfect'' solution may be good enough.  Getting the
number n of waiting threads, then sending n pulses in a row will
probably work well enough.


> static void     
> g_cond_wait_win32_impl (GCond *cond,
> 			     GMutex *mutex)
> {
>   /* XXX : what's the mutex for ? */
>   win32_check_for_error (
>     WaitForSingleObject (*(HANDLE*)cond, INFINITE));
> }

In POSIX threads, the combination of releasing a mutex, waiting for an
event and relocking the mutex can be done in two atomic steps:
	1. release a mutex
	2. wait on an event and relock the mutex

Probably, a ReleaseMutex followed by a WaitForMultipleObjects (set to
wait for all objects, and not just whichever one signals first) would
have the same or close-enough semantics.


> #define G_NSEC_PER_SEC 1000000000
>
> static gboolean
> g_cond_timed_wait_win32_impl (GCond * cond, 
> 				GMutex * entered_mutex,
> 				GTimeVal * abs_time)
> {
>   int result;
>   gboolean timed_out;
>
>   g_return_val_if_fail (cond != NULL, FALSE);
>   g_return_val_if_fail (entered_mutex != NULL, FALSE);
>
>   /* XXX : what's the mutex for ? */
>   if (!abs_time)
>     {
>       g_cond_wait (cond, entered_mutex);
>       return TRUE;
>     }
>
>   result = WaitForSingleObject (*(HANDLE*)cond,
>                                 abs_time->tv_sec * 1000 + abs_time->tv_usec / 1000);
>   timed_out = (result == WAIT_TIMEOUT);
>   if (!timed_out)
>     win32_check_for_error (result);
>   return !timed_out;
> }

As above, with the same solution since a timeout can be added to
WaitForMultipleObjects.


> static void
> g_cond_free_win32_impl (GCond * cond)
> {
>   win32_check_for_error (CloseHandle (*(HANDLE*) cond));
>   g_free (cond);
> }
>
> static GPrivate *
> g_private_new_win32_impl (GDestroyNotify destructor)
> {
>   DWORD *result = g_new (DWORD,1);
>   *result = TlsAlloc ();
>   win32_check_for_error (0xFFFFFFFF != *result);
>   return (GPrivate *) result;
> }
>
> /* NOTE: the functions g_private_get and g_private_set may not use
>    functions from gmem.c and gmessages.c */
>
> static void
> g_private_set_win32_impl (GPrivate * private_key, gpointer value)
> {
>   if (!private_key)
>     return;
>
>   win32_check_for_error (
>     TlsSetValue (*(DWORD*)private_key, value)
>     );
> }
>
> static gpointer
> g_private_get_win32_impl (GPrivate * private_key)
> {
>   if (!private_key)
>     return NULL;
>
>   return TlsGetValue (*(DWORD*)private_key);
> }
>
> static void
> g_thread_set_priority_win32_impl (gpointer thread, GThreadPriority priority)
> {
>   win32_check_for_error (SetThreadPriority (*(HANDLE*)thread,  
> 					/* XXX: g_thread_priority_map [priority] */ priority));
> }
>
> /* map the thead func signature */
> typedef struct _Win32ThreadCreateData Win32ThreadCreateData;
> struct _Win32ThreadCreateData
> {
>   GThreadFunc func;
>   gpointer data;
> };
>
> DWORD WINAPI
> ThreadFunc (LPVOID pVoid)
> {
>   Win32ThreadCreateData *create_data = (Win32ThreadCreateData *)pVoid;
>
>   create_data->func (create_data->data);
>
>   g_free (pVoid);
>   return 0;
> }

Should be static, so as not to export the symbol.  This will not
interfere with CreateThread below.


> static void
> g_thread_create_win32_impl (GThreadFunc thread_func, 
> 			      gpointer arg, 
> 			      gulong stack_size,
> 			      gboolean joinable,
> 			      gboolean bound,
> 			      GThreadPriority priority,
> 			      gpointer thread,
> 			      GError **error)
> {     
>   HANDLE hThread;
>   DWORD  dwThreadId;
>   DWORD flags = (joinable ? 0 : CREATE_SUSPENDED); /* XXX: | (bound ? 0: THR_BOUND); */
>   Win32ThreadCreateData *create_data = g_new(Win32ThreadCreateData, 1);
>   create_data->func = thread_func;
>   create_data->data = arg;
>   
>   g_return_if_fail (thread_func);
>   g_return_if_fail (priority >= G_THREAD_PRIORITY_LOW);
>   g_return_if_fail (priority <= G_THREAD_PRIORITY_URGENT);
>   
>   stack_size = MAX (g_thread_min_stack_size, stack_size);
>   
>   hThread = CreateThread (NULL, /* security attibs inherited */
>                           stack_size,
>                           &ThreadFunc,
>                           create_data,
>                           flags,  /* XXX : ??? */
>                           &dwThreadId);
>
>   if (NULL == hThread)
>     {
>       g_set_error (error, G_THREAD_ERROR, G_THREAD_ERROR_AGAIN, 
> 		   "Error creating thread: %d", GetLastError());
>       return;
>     }
>   *(HANDLE*)thread = hThread;
>
>   g_thread_set_priority_win32_impl (thread, priority);
> }

I can't event get a good idea of what THR_BOUND is.  It's not in the
Single Unix Specification v2, and a quick search on google says that it
``binds the thread to a LWP (Lightweight Process) permanently.''

Since it isn't part of the SUSv2, it isn't exactly portable from Unix
to Unix, so I wouldn't worry about it on Windows.


> static void 
> g_thread_yield_win32_impl (void)
> {
>   Sleep(0);
> }
>
> static void
> g_thread_join_win32_impl (gpointer thread)
> {
>   /* XXX: what is it ??? */
>   win32_check_for_error (0xFFFFFFFF != ResumeThread (*(HANDLE*)thread));
> }

Joining a thread means waiting for a thread to exit.  Use
WaitForSingleObject on the thread handle.


> static void 
> g_thread_exit_win32_impl (void) 
> {
>   ExitThread (0);
> }
>
> static void
> g_thread_self_win32_impl (gpointer thread)
> {
>   /*
>    * XXX: not sure if this should return the unique thread identifier
>    * from GetCurrentThreadId () instead
>    */
>
>   /*
>    * a 'pseudo' handle, not the one returned by create_thread.
>    * It could be the real handle, if we provide id->handle mapping
>    * _or_ change GLIB_SIZEOF_SYSTEM_THREAD to store both in create_thread
>    */
>   *(HANDLE*)thread = GetCurrentThread();
> }

This could be a problem.  It depends how this routine is used in
practice.  My preference would be to get rid of the pseudo-handle and
somehow return the real handle.


> static GThreadFunctions g_thread_functions_for_glib_use_default =
> {
>   g_mutex_new_win32_impl,           /* mutex */
>   g_mutex_lock_win32_impl,
>   g_mutex_trylock_win32_impl,
>   g_mutex_unlock_win32_impl,
>   g_mutex_free_win32_impl,
>   g_cond_new_win32_impl,            /* condition */
>   g_cond_signal_win32_impl,
>   g_cond_broadcast_win32_impl,
>   g_cond_wait_win32_impl,
>   g_cond_timed_wait_win32_impl,
>   g_cond_free_win32_impl,
>   g_private_new_win32_impl,         /* private thread data */
>   g_private_get_win32_impl,
>   g_private_set_win32_impl,
>   g_thread_create_win32_impl,       /* thread */
>   g_thread_yield_win32_impl,
>   g_thread_join_win32_impl,
>   g_thread_exit_win32_impl,
>   g_thread_set_priority_win32_impl,
>   g_thread_self_win32_impl
> };

Steven