Re: [PATCH]: gl_recursive_lock_init issue with pthread backend
Le vendredi 07 décembre 2007 à 12:17 +0100, Bruno Haible a écrit : Yoann Vandoorselaere wrote: I'm worried about lock initialization. If initializing a mutex fail, the application should be allowed to handle it gracefully. In all cases I know of, a lock initialization is nothing more than a memcpy. I'm not worried about it. That is great, if most implementation initialize mutex using memcpy, there is no room for failure. In this case, there is no problem checking and returning an eventual error, in case one implementation does not conform to this behavior. Some applications provide threads support only to improve performance on SMP capable hardware. If threads initialization fail, they should be given a chance to revert to single thread capability. Yes, but this is (or ought to be) handled inside the pthread_* functions. Not sure what you mean here. An example of such application would launch multiple threads in order to perform parallel computation. If for one reason or another, initialization fail, it could revert to using a single thread. I wouldn't like standard glibc pthread_mutex_lock() to call abort() for me, would you? The glibc pthread_mutex_lock() calls assert() at 8 places. Asserting is a good tool for catching obvious bug. The glibc 2.7 pthread_mutex_lock() function might return the following error from at least 14 places: - EAGAIN - EDEADLK - EOWNERDEAD - ENOTRECOVERABLE - EINVAL This doesn't account for other function called by pthread_mutex_lock() that might also trigger an error. [0] POSIX define that pthread_mutex_lock() will return EAGAIN if the maximum number of recursive locks for mutex has been exceeded., this kind of error might be expected by the programmer. glibc's intl/* code simply ignores this possibility. gnulib's 'lock' module is safer than this, it at least checks the return value. I don't see any pthread use in glibc 2.7 intl code apart from the intl/tst-gettext4.c and intl/tst-gettext5.c unit test, but anyway, other applications might want to make use of this behavior. you should let the application in control of the way it is going to handle the issue (example: there might be important cleanup to perform for persistant stored data to remain usable on next run). That's an argument I can buy. I assume that such cleanup handlers are registered as C++ exceptions on the stack, or via atexit(). Can you suggest what to do instead of abort()? Return the error, and let the application handle it, or not. If a developer does not care about handling pthread errors, nothing prevent him to use something like: assert(pthread_mutex_lock(mutex) == 0); We should let the developer define how such errors should be handled. -- Yoann Vandoorselaere | Responsable RD / CTO | PreludeIDS Technologies Tel: +33 (0)8 70 70 21 58 Fax: +33(0)4 78 42 21 58 http://www.prelude-ids.com
Re: [PATCH]: gl_recursive_lock_init issue with pthread backend
Yoann Vandoorselaere wrote: I'm worried about lock initialization. If initializing a mutex fail, the application should be allowed to handle it gracefully. In all cases I know of, a lock initialization is nothing more than a memcpy. I'm not worried about it. Some applications provide threads support only to improve performance on SMP capable hardware. If threads initialization fail, they should be given a chance to revert to single thread capability. Yes, but this is (or ought to be) handled inside the pthread_* functions. I wouldn't like standard glibc pthread_mutex_lock() to call abort() for me, would you? The glibc pthread_mutex_lock() calls assert() at 8 places. [0] POSIX define that pthread_mutex_lock() will return EAGAIN if the maximum number of recursive locks for mutex has been exceeded., this kind of error might be expected by the programmer. glibc's intl/* code simply ignores this possibility. gnulib's 'lock' module is safer than this, it at least checks the return value. you should let the application in control of the way it is going to handle the issue (example: there might be important cleanup to perform for persistant stored data to remain usable on next run). That's an argument I can buy. I assume that such cleanup handlers are registered as C++ exceptions on the stack, or via atexit(). Can you suggest what to do instead of abort()? Bruno
Re: [PATCH]: gl_recursive_lock_init issue with pthread backend
Yoann Vandoorselaere wrote: I don't know what a better handling of pthread_* function failures could look like. What do you propose? Any reason why you don't simply return error values returned by pthread_* functions (which should set errno appropriately), and let the caller possibly handle it? What could the caller do to handle it? Say, it needs access to a data structure shared among threads, and other threads are already running. In this situation, the function which should take the data structure's lock fails. What can the program do? - Proceed without locking? Good joke. - Put the current thread into an endless sleep(), and let the other threads survive as they can? Well, the other threads will likely wait on the sleeping thread at some point (because every thread has a purpose of some kind, in a real application). - Throw a C++ exception? In a C program, which does not install a handler for it, this will call ::terminate(). Not much different from abort(). - Give an error message? This is what the abort() does. Really, initializing, taking and releasing locks are so primitive operations that they *must* not fail. It's like longjmp() or 'goto' failing. Bruno
Re: [PATCH]: gl_recursive_lock_init issue with pthread backend
Le mardi 04 décembre 2007 à 09:46 +0100, Bruno Haible a écrit : Yoann Vandoorselaere wrote: I don't know what a better handling of pthread_* function failures could look like. What do you propose? Any reason why you don't simply return error values returned by pthread_* functions (which should set errno appropriately), and let the caller possibly handle it? What could the caller do to handle it? Say, it needs access to a data structure shared among threads, and other threads are already running. In this situation, the function which should take the data structure's lock fails. What can the program do? I'm worried about lock initialization. If initializing a mutex fail, the application should be allowed to handle it gracefully. Some applications provide threads support only to improve performance on SMP capable hardware. If threads initialization fail, they should be given a chance to revert to single thread capability. - Proceed without locking? Good joke. - Put the current thread into an endless sleep(), and let the other threads survive as they can? Well, the other threads will likely wait on the sleeping thread at some point (because every thread has a purpose of some kind, in a real application). - Throw a C++ exception? In a C program, which does not install a handler for it, this will call ::terminate(). Not much different from abort(). - Give an error message? This is what the abort() does. Really, initializing, taking and releasing locks are so primitive operations that they *must* not fail. It's like longjmp() or 'goto' failing. I agree that you cannot do much on critical[0] locking / unlocking error, but you should let the application in control of the way it is going to handle the issue (example: there might be important cleanup to perform for persistant stored data to remain usable on next run). I wouldn't like standard glibc pthread_mutex_lock() to call abort() for me, would you? [0] POSIX define that pthread_mutex_lock() will return EAGAIN if the maximum number of recursive locks for mutex has been exceeded., this kind of error might be expected by the programmer. -- Yoann Vandoorselaere | Responsable RD / CTO | PreludeIDS Technologies Tel: +33 (0)8 70 70 21 58 Fax: +33(0)4 78 42 21 58 http://www.prelude-ids.com
Re: [PATCH]: gl_recursive_lock_init issue with pthread backend
Le vendredi 30 novembre 2007 à 16:41 +0100, Bruno Haible a écrit : Yoann Vandoorselaere wrote: The gl_recursive_lock_init() macro used for the pthread backend never set the mutex attribute to be recursive. The end result is that a 'standard' mutex is created, which will deadlock on recursive use. A patch is attached that fixes this issue. Indeed. Thanks for the patch. Instead of a macro with a large expansion, let me create an auxiliary function in lock.c. I'm applying the patch below. Additionally, lock.h make use of the abort() function, which can be a problem if the application / library handle pthread error in a specific way. Is that done on purpose, or are you interested in a patch? It was done out of laziness, and because I don't know what a better handling of pthread_* function failures could look like. What do you propose? Any reason why you don't simply return error values returned by pthread_* functions (which should set errno appropriately), and let the caller possibly handle it? -- Yoann Vandoorselaere | Responsable RD / CTO | PreludeIDS Technologies Tel: +33 (0)8 70 70 21 58 Fax: +33(0)4 78 42 21 58 http://www.prelude-ids.com
[PATCH]: gl_recursive_lock_init issue with pthread backend
Hi,
The gl_recursive_lock_init() macro used for the pthread backend never
set the mutex attribute to be recursive. The end result is that a
'standard' mutex is created, which will deadlock on recursive use. A
patch is attached that fixes this issue.
Additionally, lock.h make use of the abort() function, which can be a
problem if the application / library handle pthread error in a specific
way. Is that done on purpose, or are you interested in a patch?
Regards,
--
Yoann Vandoorselaere | Responsable RD / CTO | PreludeIDS Technologies
Tel: +33 (0)8 70 70 21 58 Fax: +33(0)4 78 42 21 58
http://www.prelude-ids.com
diff --git a/lib/lock.h b/lib/lock.h
index 3f0da52..04d98e0 100644
--- a/lib/lock.h
+++ b/lib/lock.h
@@ -361,11 +361,26 @@ typedef pthread_mutex_t gl_recursive_lock_t;
PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
# endif
# define gl_recursive_lock_init(NAME) \
- do \
-{ \
- if (pthread_in_use () pthread_mutex_init (NAME, NULL) != 0) \
-abort (); \
-} \
+ do \
+{\
+ pthread_mutexattr_t mattr; \
+ \
+ if ( ! pthread_in_use() ) \
+return 0;\
+ \
+ if (pthread_mutexattr_init (mattr) != 0) \
+abort ();\
+ \
+ if (pthread_mutexattr_settype(mattr, PTHREAD_MUTEX_RECURSIVE) != 0) { \
+pthread_mutexattr_destroy(mattr); \
+abort ();\
+ } \
+ \
+ if (pthread_mutex_init (NAME, mattr) != 0) \
+abort ();\
+ \
+ pthread_mutexattr_destroy(mattr); \
+}\
while (0)
# define gl_recursive_lock_lock(NAME) \
do\
Re: [PATCH]: gl_recursive_lock_init issue with pthread backend
Yoann Vandoorselaere wrote:
The gl_recursive_lock_init() macro used for the pthread backend never
set the mutex attribute to be recursive. The end result is that a
'standard' mutex is created, which will deadlock on recursive use. A
patch is attached that fixes this issue.
Indeed. Thanks for the patch. Instead of a macro with a large expansion,
let me create an auxiliary function in lock.c. I'm applying the patch below.
Additionally, lock.h make use of the abort() function, which can be a
problem if the application / library handle pthread error in a specific
way. Is that done on purpose, or are you interested in a patch?
It was done out of laziness, and because I don't know what a better handling
of pthread_* function failures could look like. What do you propose?
Bruno
2007-11-30 Bruno Haible [EMAIL PROTECTED]
* lib/lock.h (gl_recursive_lock_init) [PTHREAD
PTHREAD_RECURSIVE_MUTEX_INITIALIZER]: Call
glthread_recursive_lock_init.
* lib/lock.c (glthread_recursive_lock_init)
[PTHREAD_RECURSIVE_MUTEX_INITIALIZER]: New function.
Reported by Yoann Vandoorselaere [EMAIL PROTECTED].
--- lib/lock.h
+++ lib/lock.h
@@ -361,11 +361,11 @@ typedef pthread_mutex_t gl_recursive_lock_t;
PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
# endif
# define gl_recursive_lock_init(NAME) \
- do \
-{ \
- if (pthread_in_use () pthread_mutex_init (NAME, NULL) != 0) \
-abort (); \
-} \
+ do \
+{ \
+ if (pthread_in_use ()) \
+glthread_recursive_lock_init (NAME); \
+} \
while (0)
# define gl_recursive_lock_lock(NAME) \
do\
@@ -388,6 +388,7 @@ typedef pthread_mutex_t gl_recursive_lock_t;
abort (); \
} \
while (0)
+extern void glthread_recursive_lock_init (gl_recursive_lock_t *lock);
# else
--- lib/lock.c
+++ lib/lock.c
@@ -1,5 +1,5 @@
/* Locking in multithreaded situations.
- Copyright (C) 2005-2006 Free Software Foundation, Inc.
+ Copyright (C) 2005-2007 Free Software Foundation, Inc.
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -250,7 +250,24 @@ glthread_rwlock_destroy (gl_rwlock_t *lock)
# if HAVE_PTHREAD_MUTEX_RECURSIVE
-# if !(defined PTHREAD_RECURSIVE_MUTEX_INITIALIZER || defined
PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP)
+# if defined PTHREAD_RECURSIVE_MUTEX_INITIALIZER || defined
PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
+
+void
+glthread_recursive_lock_init (gl_recursive_lock_t *lock)
+{
+ pthread_mutexattr_t attributes;
+
+ if (pthread_mutexattr_init (attributes) != 0)
+abort ();
+ if (pthread_mutexattr_settype (attributes, PTHREAD_MUTEX_RECURSIVE) != 0)
+abort ();
+ if (pthread_mutex_init (lock, attributes) != 0)
+abort ();
+ if (pthread_mutexattr_destroy (attributes) != 0)
+abort ();
+}
+
+# else
void
glthread_recursive_lock_init (gl_recursive_lock_t *lock)
