RE: [boost] thread safety in boost date time
Regarding the thread safety issue relating to getenv() in boost date_time, we have now identified the problem and it appears to be a call to putenv() elsewhere in log4cxx. putenv() is usually not bad, but here there is a call to putenv() with a string created on the stack. When the thread ends, the stack gets destroyed and further calls to getenv() cause an access violation. So the bug was not thread safety, but simply an ugly bug in log4cxx. However, the fact remains that the date_time library uses non-thread safe functions in a way that cannot be controlled by the user. boost date_time may not be the only one calling these functions, and so "wrap it in a mutex on your side" requires the user to know which calls to wrap in a mutex, and be sure that all third parties, not just boost but also log4cxx etc., using "localtime" are wrapped in the same mutex. And this assumes that all that is necessary to solve the thread safety in some implementation of "localtime" is to call "localtime" serially and never concurrently. This is operating system dependent and one of the reasons to use boost is to eliminate os-dependent code. log4cxx has fixed this already in its cvs due to a rewrite of the framework. When I mentioned it should be in a central place, this is because there are various libraries, such as spirit, that require (or required -- we're using 1.30.2) BOOST_SPIRIT_THREAD_SAFE to be defined, and so it would be nice if all "what you need to do to work in multi- threaded environments" was all in one central place, rather than forcing the user to collect bits of information on this issue from each of the various libraries. Yitzhak Sapir
On Sun, 13 Mar 2005 12:43:37 +0200, Yitzhak Sapir wrote
Regarding the thread safety issue relating to getenv() in boost date_time, we have now identified the problem and it appears to be a call to putenv() elsewhere in log4cxx. putenv() is usually not bad, but here there is a call to putenv() with a string created on the stack. When the thread ends, the stack gets destroyed and further calls to getenv() cause an access violation. So the bug was not thread safety, but simply an ugly bug in log4cxx.
Thanks for the update on the problem.
However, the fact remains that the date_time library uses non-thread safe functions in a way that cannot be controlled by the user. ...snip details...
Yes, as I said earlier it's on the todo list for 1.33. BTW, worst case is that we could make our own localtime_r function in the clock classes.
When I mentioned it should be in a central place, this is because there are various libraries, such as spirit, that require (or required -- we're using 1.30.2) BOOST_SPIRIT_THREAD_SAFE to be defined, and so it would be nice if all "what you need to do to work in multi- threaded environments" was all in one central place, rather than forcing the user to collect bits of information on this issue from each of the various libraries.
I see. So is BOOST_HAS_THREADS or BOOST_DISABLE_THREADS the correct macro for libraries to use? Seems like if DISABLE_THREADS is set then we could safely call the non-reentrant versions. Jeff
Jeff Garland wrote:
I see. So is BOOST_HAS_THREADS or BOOST_DISABLE_THREADS the correct macro for libraries to use?
In theory, BOOST_HAS_THREADS is set by the config system if it detects a multithreaded platform, and BOOST_DISABLE_THREADS is set by the user to prevent this from happening. In practice, BOOST_DISABLE_THREADS is being automatically set from time to time, but I'm not sure whether one can rely on that.
On Sun, 13 Mar 2005 18:52:54 +0200, Peter Dimov wrote
Jeff Garland wrote:
I see. So is BOOST_HAS_THREADS or BOOST_DISABLE_THREADS the correct macro for libraries to use?
In theory, BOOST_HAS_THREADS is set by the config system if it detects a multithreaded platform, and BOOST_DISABLE_THREADS is set by the user to prevent this from happening.
In practice, BOOST_DISABLE_THREADS is being automatically set from time to time, but I'm not sure whether one can rely on that.
Ok, thx -- just looking at the config code confirms that if DISABLE_THREADS is set then HAS_THREADS gets unset -- so HAS_THREADS looks like the way to go. I'm going to modify the config docs for BOOST_DISABLE_THREADS to make this clearer if no one objects (just adding a second sentence...). Currently reads: When defined, disables threading support, even if the compiler in its current translation mode supports multiple threads. Suggested: When defined, disables threading support, even if the compiler in its current translation mode supports multiple threads. Normally this macro is defined by the user in the BOOST_ROOT/config/user.hpp or via a compile option switch. Jeff
Currently reads: When defined, disables threading support, even if the compiler in its current translation mode supports multiple threads.
Suggested: When defined, disables threading support, even if the compiler in its current translation mode supports multiple threads. Normally this macro is defined by the user in the BOOST_ROOT/config/user.hpp or via a compile option switch.
Please do go ahead with that. Thanks, John.
On Mon, 14 Mar 2005 16:30:51 -0000, John Maddock wrote
Currently reads: When defined, disables threading support, even if the compiler in its current translation mode supports multiple threads.
Suggested: When defined, disables threading support, even if the compiler in its current translation mode supports multiple threads. Normally this macro is defined by the user in the BOOST_ROOT/config/user.hpp or via a compile option switch.
Please do go ahead with that.
Done. Jeff
Jeff Garland wrote:
However, the fact remains that the date_time library uses non-thread safe functions in a way that cannot be controlled by the user. ...snip details...
Yes, as I said earlier it's on the todo list for 1.33. BTW, worst case is that we could make our own localtime_r function in the clock classes.
I'm not sure about this, but I think that the windows multi-threaded C libraries use thread specific storage for functions like 'localtime'. So on that platfrom, they're okay.
On Sun, 13 Mar 2005 17:31:08 +0000, Daniel James wrote
Jeff Garland wrote:
However, the fact remains that the date_time library uses non-thread safe functions in a way that cannot be controlled by the user. ...snip details...
Yes, as I said earlier it's on the todo list for 1.33. BTW, worst case is that we could make our own localtime_r function in the clock classes.
I'm not sure about this, but I think that the windows multi-threaded C libraries use thread specific storage for functions like 'localtime'. So on that platfrom, they're okay.
My impresssion as well, although I haven't double checked... Jeff
I see. So is BOOST_HAS_THREADS or BOOST_DISABLE_THREADS the correct macro for libraries to use? Seems like if DISABLE_THREADS is set then we could safely call the non-reentrant versions.
Use BOOST_HAS_THREADS: it's set only if the platform has threading support, the compiler is in thread safe mode, and BOOST_DISABLE_THREADS is *not* set. John.
On Sun, 13 Mar 2005 12:43:37 +0200, Yitzhak Sapir wrote
...snip...
However, the fact remains that the date_time library uses non-thread safe functions in a way that cannot be controlled by the user. boost date_time may not be the
I've checked changes into CVS which will call localtime_r or the appropriate equivalent for the various clock classes when BOOST_HAS_THREADS is set. Jeff ---- Checking in c_local_time_adjustor.hpp; new revision: 1.7; previous revision: 1.6 Checking in c_time.hpp; new revision: 1.5; previous revision: 1.4 Checking in date_clock_device.hpp; new revision: 1.5; previous revision: 1.4 Checking in microsec_time_clock.hpp; new revision: 1.13; previous revision: 1.12 Checking in time_clock.hpp; new revision: 1.8; previous revision: 1.7
participants (5)
-
Daniel James -
Jeff Garland -
John Maddock -
Peter Dimov -
Yitzhak Sapir