From: SourceForge.net <no...@so...> - 2004-10-06 22:38:07
|
Bugs item #992108, was opened at 2004-07-15 20:52 Message generated for change (Comment added) made by davygrvy You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=992108&group_id=10894 Category: 47. Threading Group: None Status: Open Resolution: None Priority: 5 Submitted By: Joe Mistachkin (mistachkin) Assigned to: Zoran Vasiljevic (vasiljevic) Summary: TclFinalizeLock on Unix not correct. Initial Comment: The function currently reads: void TclFinalizeLock () { #ifdef TCL_THREADS /* * You do not need to destroy mutexes that were created with the * PTHREAD_MUTEX_INITIALIZER macro. These mutexes do not need * any destruction: masterLock, allocLock, and initLock. */ pthread_mutex_unlock(&initLock); #endif } I believe that both the comment and the code are incorrect. Mutexes created with the pthread library have memory associated with them internally when you call pthread_mutex_create, therefore, pthread_mutex_destroy must be called on them. In my opinion, the function should be more like this: TclFinalizeLock () { #ifdef TCL_THREADS /* * These mutexes need to be destroyed via pthread_mutex_destroy * because they were created with pthread_mutex_create, which * internally allocates memory on some platforms. */ pthread_mutex_destroy(&allocLock); pthread_mutex_destroy(&masterLock); pthread_mutex_unlock(&initLock); pthread_mutex_destroy(&initLock); #endif } ---------------------------------------------------------------------- >Comment By: David Gravereaux (davygrvy) Date: 2004-10-06 15:38 Message: Logged In: YES user_id=7549 andreas_kupries wrote: > David Gravereaux's opinion is important as welll IMHO. I'll just watch as this works itself out :) I did do my homework reading the POSIX docs, but it is fun to see how the implementation behind it, isn't following the documentation. ---------------------------------------------------------------------- Comment By: Zoran Vasiljevic (vasiljevic) Date: 2004-07-20 02:55 Message: Logged In: YES user_id=95086 To: "assume that all threads are destroyed" is not that easy. Who is assuming that all threads are destroyed? You can very easily have apps using Tcl API which do create threads using Tcl_CreateThread and Tcl has no notion of those whatsoever. Therefore, assuming that all threads are destroyed at the Tcl_Finalize, except for the main thread is not always true. As far as I can see, destroying of initLock is troublesome and can lead (in certain circumstances) to problems. I think in this case we need to make a trade-off. Either the caller *never* calls Tcl_Finalize from two threads at the same time, or we accept a single mutex leak on FreeBSD. The way the Tcl_Finalize is now implemented would easily allow any number of threads to simultaneously call the Tcl_Finalize w/o stepping on each other toes. Forcefully deleting the initLock brings a significant limitation here. I would suggest, we can bring this topic to the Tcl core list and ask other people what do to in this case: either force Tcl_Finalize to really clean up *everything* risking potential cores or allowing one leaked mutex on FreeBsd implementation. I can't make this decision alone given the possible side-effects. ---------------------------------------------------------------------- Comment By: Joe Mistachkin (mistachkin) Date: 2004-07-17 13:00 Message: Logged In: YES user_id=113501 Actually, I don't really think it's an issue because it is done dead-last in the finalization process and various other things before it assume that all threads are destroyed, etc. The Win32 code simply deletes the locks while they are being held (not really recommended). What would be ideal here (and potentially quite a few other places in the core) is a spinlock that doesn't have to be deleted and can protect the mutexes. ---------------------------------------------------------------------- Comment By: Zoran Vasiljevic (vasiljevic) Date: 2004-07-17 11:01 Message: Logged In: YES user_id=95086 Uhuhuhu... voodoo (mutex) magic! You better check the pthread_mutex_lock implementation for what's gonna happen in the second thread which is sleeping on the initLock while you're NULL-ing it under its very feet :-) ---------------------------------------------------------------------- Comment By: Joe Mistachkin (mistachkin) Date: 2004-07-17 10:48 Message: Logged In: YES user_id=113501 Maybe something like this (init lock is held): pthread_mutex_t tempInitLock; pthread_mutex_t tempMasterLock; pthread_mutex_t tempAllocLock; tempInitLock = initLock; initLock = NULL; tempMasterLock = masterLock; masterLock = NULL; tempAllocLock = allocLock; allocLock = NULL; pthread_mutex_destroy(&tempAllocLock); pthread_mutex_destroy(&tempMasterLock); pthread_mutex_unlock(&tempInitLock); pthread_mutex_destroy(&tempInitLock); ---------------------------------------------------------------------- Comment By: Zoran Vasiljevic (vasiljevic) Date: 2004-07-17 03:37 Message: Logged In: YES user_id=95086 OK. The remaining thing to be resolved here is: pthread_mutex_unlock(&initLock); pthread_mutex_destroy(&initLock); Theoretically, you have a classcal race condition here. If two threads attempt to call Tcl_Finalize, at the moment you release the initLock mutex in the first thread, the other thread starts the calling TclFinalizeLock() and you go thru the call twice, destroying the already destroyed allocLock, masterLock and initLock mutexes. This is very tricky to solve correctly. You may do pthread_mutex_destroy(&allocLock), allocLock = NULL; pthread_mutex_destroy(&masterLock), masterLock = NULL; to avoid these two traps (in which case current freebsd implementation will return EINVAL) but you're left with the potential hazardous initLock where you can go and hit the destroyed but not re-initialized mutex and definitely get the segmentation at the line: _SPINLOCK(&(*mutex)->lock); in pthread_destroy_mutex() In order to avoid this, you must get yet another mutex and the story repeats. Eventually, you must at least accept one leaked mutex (the initLock). I see no other possibility. Do you? ---------------------------------------------------------------------- Comment By: Joe Mistachkin (mistachkin) Date: 2004-07-16 15:58 Message: Logged In: YES user_id=113501 I suggest that we #ifdef __FreeBSD__ for now and put a large comment there that indicates that if you find a leak report it and we will add that platform to the #ifdef as a platform that needs the mutexes freed. ---------------------------------------------------------------------- Comment By: Zoran Vasiljevic (vasiljevic) Date: 2004-07-16 15:45 Message: Logged In: YES user_id=95086 Interesting.... Agree, for freebsd you will get the leak. So, we have now 3 cases covered. For freebsd, you can/ must/should destroy static mutexes, for Linux/Solaris you need not do that and if you do, there is no harm. The question is how are we going to conditionaly configure and wrap this in an elegant way? We must also take in the consideration that for some platforms (AIX, HP, Win) we have no access to sources to look at and vendor docs are not very helpful. Seems like quite a lot of work to me. You have any magic idea? ---------------------------------------------------------------------- Comment By: Joe Mistachkin (mistachkin) Date: 2004-07-16 15:12 Message: Logged In: YES user_id=113501 The following code is from FreeBSD 4.10-STABLE. -- in /usr/src/include/pthread.h: #define PTHREAD_MUTEX_INITIALIZER NULL -- in /usr/src/lib/libc_r/uthread/uthread_mutex.c: int _pthread_mutex_init(pthread_mutex_t * mutex, const pthread_mutexattr_t * mutex_attr) { enum pthread_mutextype type; int protocol; int ceiling; pthread_mutex_t pmutex; int ret = 0; if (mutex == NULL) ret = EINVAL; /* Check if default mutex attributes: */ else if (mutex_attr == NULL || *mutex_attr == NULL) { /* Default to a (error checking) POSIX mutex: */ type = PTHREAD_MUTEX_ERRORCHECK; protocol = PTHREAD_PRIO_NONE; ceiling = PTHREAD_MAX_PRIORITY; } /* Check mutex type: */ else if (((*mutex_attr)->m_type < PTHREAD_MUTEX_ERRORCHECK) || ((*mutex_attr)->m_type >= MUTEX_TYPE_MAX)) /* Return an invalid argument error: */ ret = EINVAL; /* Check mutex protocol: */ else if (((*mutex_attr)->m_protocol < PTHREAD_PRIO_NONE) || ((*mutex_attr)->m_protocol > PTHREAD_MUTEX_RECURSIVE)) /* Return an invalid argument error: */ ret = EINVAL; else { /* Use the requested mutex type and protocol: */ type = (*mutex_attr)->m_type; protocol = (*mutex_attr)->m_protocol; ceiling = (*mutex_attr)->m_ceiling; } /* Check no errors so far: */ if (ret == 0) { if ((pmutex = (pthread_mutex_t) malloc(sizeof(struct pthread_mutex))) == NULL) ret = ENOMEM; else { /* Reset the mutex flags: */ pmutex->m_flags = 0; /* Process according to mutex type: */ switch (type) { /* case PTHREAD_MUTEX_DEFAULT: */ case PTHREAD_MUTEX_ERRORCHECK: case PTHREAD_MUTEX_NORMAL: /* Nothing to do here. */ break; /* Single UNIX Spec 2 recursive mutex: */ case PTHREAD_MUTEX_RECURSIVE: /* Reset the mutex count: */ pmutex->m_data.m_count = 0; break; /* Trap invalid mutex types: */ default: /* Return an invalid argument error: */ ret = EINVAL; break; } if (ret == 0) { /* Initialise the rest of the mutex: */ TAILQ_INIT(&pmutex->m_queue); pmutex->m_flags |= MUTEX_FLAGS_INITED; pmutex->m_owner = NULL; pmutex->m_type = type; pmutex->m_protocol = protocol; pmutex->m_refcount = 0; if (protocol == PTHREAD_PRIO_PROTECT) pmutex->m_prio = ceiling; else pmutex->m_prio = 0; pmutex->m_saved_prio = 0; _MUTEX_INIT_LINK(pmutex); memset(&pmutex->lock, 0, sizeof(pmutex->lock)); *mutex = pmutex; } else { free(pmutex); *mutex = NULL; } } } /* Return the completion status: */ return(ret); } int _pthread_mutex_destroy(pthread_mutex_t * mutex) { int ret = 0; if (mutex == NULL || *mutex == NULL) ret = EINVAL; else { /* Lock the mutex structure: */ _SPINLOCK(&(*mutex)->lock); /* * Check to see if this mutex is in use: */ if (((*mutex)->m_owner != NULL) || (TAILQ_FIRST(&(*mutex)->m_queue) != NULL) || ((*mutex)->m_refcount != 0)) { ret = EBUSY; /* Unlock the mutex structure: */ _SPINUNLOCK(&(*mutex)->lock); } else { /* * Free the memory allocated for the mutex * structure: */ _MUTEX_ASSERT_NOT_OWNED(*mutex); free(*mutex); /* * Leave the caller's pointer NULL now that * the mutex has been destroyed: */ *mutex = NULL; } } /* Return the completion status: */ return (ret); } ---------------------------------------------------------------------- Comment By: Zoran Vasiljevic (vasiljevic) Date: 2004-07-16 15:10 Message: Logged In: YES user_id=95086 Just out of the couriosity... I've tested Solaris and Linux, both NPTL and linuxthreads. No implementation does any dynamic memory allocations on mutexes. They are either dynmically allocated by the user, in which case they need to be deallocated by the user as well, or they are staticaly allocated by the compiler in which case there is nothing to deallocate. For those two platforms, your suggested change won't lead to any problem. So, at least we have two of them. You can look into the freebsd stuff. ---------------------------------------------------------------------- Comment By: Zoran Vasiljevic (vasiljevic) Date: 2004-07-16 14:57 Message: Logged In: YES user_id=95086 Frankly speaking, the common sense tells that if you first destroy the thing, then you need to create the thing before you can use it again. So, if you destroy the static mutex (which you did not create at the first place, rather the compiler did that for you) who is going to create it? This is very implementation specific. For example, have a look on the NPTL pthread_mutex_destroy: int __pthread_mutex_destroy (mutex) pthread_mutex_t *mutex; { if (mutex->__data.__nusers != 0) return EBUSY; return 0; } I am all with you that we should aim at cleaning up what's possible after us, but I'm afraid that in some circumstances we might clean-up too much. To be on the safe side, I'd rather risk a leak then a (eventual) memory corruption. Ideally, one would grab sources of all thread library implementations, look what is really done under the hood and make a wrapper with ifdef's for all supported variants. This is hard job to do. And implementor docs are more than confusing in that respect, as Andreas have found out. ---------------------------------------------------------------------- Comment By: Joe Mistachkin (mistachkin) Date: 2004-07-16 14:17 Message: Logged In: YES user_id=113501 If you unload the Tcl library and reload it, the memory will be leaked by pthread_mutex_init for all these mutexes (verified under valgrind on FreeBSD). And quite frankly, since Tcl is supposed to be fully supported in application embedding scenerios, I do not agree with the assertion that memory and resources (both internal and external) should not be reclaimed whenever possible. Is there any platform where calling pthread_mutex_destroy would cause any harm in this case? On Windows, this would be analogous to not calling DeleteCriticalSection on the same three locks (which are also statically allocated). ---------------------------------------------------------------------- Comment By: Zoran Vasiljevic (vasiljevic) Date: 2004-07-16 14:00 Message: Logged In: YES user_id=95086 I understand your (symetric) reasoning very well. But... Let us forget memory leaks at this place and think little bit differently... Say, you were to destroy static mutexes on Tcl_Finalize(), that is, on unload of Tcl library, and then load the same Tcl library *again* in the same process space. What is going to happen when you try to access one of those mutexes (which is absolutely certain, per-se) again? Hm? Therefore, I recommend you not to touch them, really. Just let them live their static life. Besides, there is absolutely no issue with memory leaks here. Those are just part of the process memory and will be reclaimed when the process exits. HTH ---------------------------------------------------------------------- Comment By: Andreas Kupries (andreas_kupries) Date: 2004-07-16 11:18 Message: Logged In: YES user_id=75003 The macro PTHREAD_MUTEX_INITIALIZER does not seem to use 'pthread_mutex_create' (Linux). ... Googling. I found this http://nf.apac.edu.au/facilities/sc/compaq_mirror/HTML/ARH9RBTE/DOCU0022.HTM Docs for "True64 Unix". In the description of 'pthread_mutex_init' I find: <quote> Use the PTHREAD_MUTEX_INITIALIZER macro to statically initialize a mutex without calling this routine. Statically initialized mutexes need not be destroyed using pthread_mutex_destroy() </quote> The opengroup has http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_mutex_init.html which says <quote> In cases where default mutex attributes are appropriate, the macro PTHREAD_MUTEX_INITIALIZER can be used to initialise mutexes that are statically allocated. The effect is equivalent to dynamic initialisation by a call to pthread_mutex_init() with parameter attr specified as NULL, except that no error checks are performed. </quote> For Java http://java.icmc.sc.usp.br/library/books/ibm_pthreads/users-61.htm <quote> Mutex initialization using the PTHREAD_MUTEX_INITIALIZER does not immediately initialize the mutex. Instead, on first use, the pthread_mutex_lock() or pthread_mutex_trylock() functions branch into a slow path and cause the initialization of the mutex. Because a mutex isn't just a simple memory object, and requires that some resources be allocated by the system, an attempt to call pthread_mutex_destroy() or pthread_mutex_unlock() on a mutex that has was statically initialized using PTHREAD_MUTEX_INITIALER and was not yet locked will result in an EINVAL error. </quote> So for at least some platforms this is publically documented behaviour, and for others the interaction is not really talked about, more implicit. And for some you have to call the destroy(), it seems. Confusing. Reassigning to Zoran for his opinion. David Gravereaux's opinion is important as welll IMHO. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=992108&group_id=10894 |