[Serialization] Thread-unsafe singleton
Robert, I've seen this thread here: http://thread.gmane.org/gmane.comp.lib.boost.user/21885 And I believe I am coming across the same issue (static *serializer instances) manifesting itself as an assertion failure: Assertion failed: lookup(eti) == m_self->m_map.end(), file vendor/boost/libs/serialization/src/extended_type_info.cpp, line 77 That assertion means that a type_info key is being registered twice. I've verified this via cout (\o/ for debugging). Now I am also of the opinion that there is no way to do this initialization without bringing a threading library into Boost.Serialization. From reading the comments in the code, it appears that the only reason the library is using these singletons is because of VC6. Is it possible to have some thread-safe way (perhaps via ifdefs?) to do these singletons? Or do you think there is another reason I am having this issue? Thanks, Sohail PS: I'm using 1.33.1. If the issue is fixed in 1.34, I'd be very glad!
I believe the issue has been addressed in 1.34. That may or may not be the same as "fixed in 1.34". I've tried to address it by: a) fixing the order of initialization by carefully considering depencies. b) permiting multiple instances of extended type info for the same type which might occur in diffferent DLLS. The entails deleting the correct one when the destructor is called. Its hard to make a test for so its sort of precarious. But there it is. Robert Ramey Sohail Somani wrote:
Robert,
I've seen this thread here:
http://thread.gmane.org/gmane.comp.lib.boost.user/21885
And I believe I am coming across the same issue (static *serializer instances) manifesting itself as an assertion failure:
Assertion failed: lookup(eti) == m_self->m_map.end(), file vendor/boost/libs/serialization/src/extended_type_info.cpp, line 77
That assertion means that a type_info key is being registered twice. I've verified this via cout (\o/ for debugging).
Now I am also of the opinion that there is no way to do this initialization without bringing a threading library into Boost.Serialization. From reading the comments in the code, it appears that the only reason the library is using these singletons is because of VC6. Is it possible to have some thread-safe way (perhaps via ifdefs?) to do these singletons? Or do you think there is another reason I am having this issue?
Thanks,
Sohail
PS: I'm using 1.33.1. If the issue is fixed in 1.34, I'd be very glad!
-----Original Message----- From: boost-users-bounces@lists.boost.org [mailto:boost-users-bounces@lists.boost.org] On Behalf Of Robert Ramey Sent: Monday, May 21, 2007 7:40 PM To: boost-users@lists.boost.org Subject: Re: [Boost-users] [Serialization] Thread-unsafe singleton
I believe the issue has been addressed in 1.34. That may or may not be the same as "fixed in 1.34". I've tried to address it by:
a) fixing the order of initialization by carefully considering depencies.
b) permiting multiple instances of extended type info for the same type which might occur in diffferent DLLS. The entails deleting the correct one when the destructor is called.
Its hard to make a test for so its sort of precarious. But there it is.
Robert, Thanks for your quick response. Could you point me to the files that have been changed to address the issues? In any case, I've been able to reproduce it pretty easily on my side and am working on disentangling the test from my project. Would you be interested if I can extract a test case? Thanks, Sohail
Sohail Somani wrote:
Thanks for your quick response. Could you point me to the files that have been changed to address the issues?
You'll have to browse the CVS - look into extended type info
In any case, I've been able to reproduce it pretty easily on my side and am working on disentangling the test from my project. Would you be interested if I can extract a test case?
Maybe. Robert Ramey
On Mon, 2007-05-21 at 20:04 -0700, Robert Ramey wrote:
Sohail Somani wrote:
Thanks for your quick response. Could you point me to the files that have been changed to address the issues?
You'll have to browse the CVS - look into extended type info
I am looking at: http://boost.cvs.sourceforge.net/boost/boost/libs/serialization/src/extended_type_info.cpp?r1=1.11&r2=1.12 which I assume is the change you are suggesting. This still doesn't address the thread-safety issues inherent in Myers Singleton that is used here: http://boost.cvs.sourceforge.net/boost/boost/boost/serialization/extended_type_info_typeid.hpp?revision=1.9&view=markup on line 96. With the above code, it is possible to have double initialization of the extended_type_info derived instances and then eventually double initialization of detail::*map which could be a more dramatic error. So if anyone uses BOOST_CLASS_EXPORT() you cannot reason about your program's thread-safety. I was sent a patch privately by someone who changed the instance in the first link to be a class member static with a caveat that it invokes "dynamic initialization." I am new to the whole C++ language lawyer thing but in section 3.6.3(.3) of the standard, it says: It is implementation-defined whether or not the dynamic initialization (8.5, 9.4, 12.1, 12.6.1) of an object of namespace scope is done before the first statement of main. So I really don't think there is any way to do this lazy initialization portably in the presence of threads (threads? what are those?) However, the choice to delay it to a function-scope static makes it easier to make it work. I can develop a patch using boost once that I think would work. Locally, I'm going to have to do this anyway. Fortunately, my test has ceased to fail so I can't test it. To make it as "pay for what you use", one can #ifdef it out if they have special knowledge of the platform/compiler. Suggestions, comments or flames? All three are appreciated. Sohail
Sohail Somani wrote:
I can develop a patch using boost once that I think would work. Locally, I'm going to have to do this anyway. Fortunately, my test has ceased to fail so I can't test it. To make it as "pay for what you use", one can #ifdef it out if they have special knowledge of the platform/compiler.
OK - you've convinced me there could be a problem. I don't see a magic bullet solution. I think users need a couple of options and a good explanation in the manual. Options a) don't use export - use explicit archive level registration instead. b) some sort of explicit type registration. Users could just put at after the main entry point and/or in the DLL loading phase c) an automatic method using thread safety primitives - maybe there are some small ones. Then you have the *#ifdef ... The main obstacle is that requires a detailed understandable explanation along with examples - a fair amount of work. Note that this really applies to all extended type info systems so the explanation is even more complex. And once embarks upon this kind of effort, its interesting to consider "finishing" extended type info with some missing functionality like "construct from export name". It would also be interesting to investigate if there is some sort of lock free solution - At least on some compilers/ processors. Just goes to show that no good idea goes unpunished. SOOO I'm am positive about the idea. However, like most good ideas, its a lot bigger than it first appears. Having written this, it would seem that this is territory well traveled by those working on a good Singleton suitable for such a purpose. (Amazingly, I don't even know if boost has such a thiing) Ideally, I would just like to use that (assuming it doesn't import everything on planet earth). Robert Ramey
To those of us just joining the thread (boost developers list), we are discussing the use of Myers Singleton in the Boost.Serialization code and it's ramifications for thread-safety. You can see the thread here: http://thread.gmane.org/gmane.comp.lib.boost.user/27457 On Wed, 2007-05-23 at 22:38 -0700, Robert Ramey wrote:
Sohail Somani wrote:
I can develop a patch using boost once that I think would work. Locally, I'm going to have to do this anyway. Fortunately, my test has ceased to fail so I can't test it. To make it as "pay for what you use", one can #ifdef it out if they have special knowledge of the platform/compiler.
OK - you've convinced me there could be a problem.
I don't see a magic bullet solution. I think users need a couple of options and a good explanation in the manual.
Options
a) don't use export - use explicit archive level registration instead. b) some sort of explicit type registration. Users could just put at after the main entry point and/or in the DLL loading phase
How is (b) different from (a)? Is this related to your "load/save by name?"
c) an automatic method using thread safety primitives - maybe there are some small ones. Then you have the *#ifdef ...
There is a very small one ;-) The idea is here (copied and tweaked to
protect the innocent - may not compile):
// thread-safe lazily initialized singleton
template<typename T>
struct singleton
{
public:
static T & instance()
{
boost::call_once(call_once,once_); // the magic happens here
return get_instance();
}
private:
<*structors>;
// call_once *MUST* be called before calling this function
static T & get_instance()
{
static T object;
return object;
}
static void call_once()
{
get_instance();
}
static boost::once_flag once_;
};
// guaranteed to be called before main since it uses initialization
// with a constant expression (3.6.2.1) - language lawyers help please.
// Also, what happens with dlopen'ish stuff???? This is heavily platform
// specific. Someone save us from this mess.
template<typename T>
boost::once_flag singleton<T>::once_ = BOOST_ONCE_INIT;
Then usage would be something like:
static extended_type_info *
get_instance()
{
return &(singleton
The main obstacle is that requires a detailed understandable explanation along with examples - a fair amount of work. Note that this really applies to all extended type info systems so the explanation is even more complex.
Well I would have to rely on you to tell me where we need to apply the fixes. I would be happy to add some documentation to a patch against 1.34.
And once embarks upon this kind of effort, its interesting to consider "finishing" extended type info with some missing functionality like "construct from export name".
I have no idea why the two affect each other!
It would also be interesting to investigate if there is some sort of lock free solution - At least on some compilers/ processors.
I'm not going to touch lock-free lazily initialized singletons! I'd rather have my code work ;-)
Just goes to show that no good idea goes unpunished.
:-)
SOOO I'm am positive about the idea. However, like most good ideas, its a lot bigger than it first appears.
Don't see why myself. In multi-threaded builds of serialization, we link with Boost.Thread and use boost::once.
Having written this, it would seem that this is territory well traveled by those working on a good Singleton suitable for such a purpose.
I've worked on the above singleton, tested it on a few x86 OSes and it seems to work. But I would be very happy if someone has a better solution that is tested.
(Amazingly, I don't even know if boost has such a thiing) Ideally, I would just like to use that (assuming it doesn't import everything on planet earth).
I believe that Boost has everything you need for a *correct* solution. Sohail
On Thu, 2007-05-24 at 11:03 -0700, Sohail Somani wrote:
On Wed, 2007-05-23 at 22:38 -0700, Robert Ramey wrote:
Sohail Somani wrote:
SOOO I'm am positive about the idea. However, like most good ideas, its a lot bigger than it first appears.
Don't see why myself. In multi-threaded builds of serialization, we link with Boost.Thread and use boost::once.
Actually, after reflecting a bit more on the code, there are a lot of places in the serialization which are susceptible to race conditions. In extended_type_info.cpp, all the mutating static functions have a race condition. So it *is* a bigger issue than I previously thought, like you say. I wonder if its more work to fix the issues or to resort to manual registration. I think even locally it is better to fix the issues, but then a lot of testing needs to be done. The easiest thing is to have a boost mutex around the mutating functions (which of course cannot be statically initialized...) I'll have to talk with my team to see what the best course is for us. Thanks for your replies, Sohail
Sohail Somani wrote:
Options
a) don't use export - use explicit archive level registration instead. b) some sort of explicit type registration. Users could just put at after the main entry point and/or in the DLL loading phase
How is (b) different from (a)? Is this related to your "load/save by name?"
The issue is the invocation of a function which impliles the construction of the required type. This occurs as a side effect of serializing the type in an archive. But it could ust as well be done explicitly before any threads are started. This explanation is much confused by the fact that I've used "registration" to mean more than one thing. One thing is the creation of the suitable extended_type_info object (which is being discussed here). while another thing is the construction of certain serialization objects which occur for every combination of archive type and serializable type which is serialized through a pointer. This latter is also done as a side effect of just using serialization.
And once embarks upon this kind of effort, its interesting to consider "finishing" extended type info with some missing functionality like "construct from export name".
I have no idea why the two affect each other!
Your idea is to refine, and improve extended_type_info. It has utility outside the serialization package. However to make this as useful as it should be would require adding some "missing" functions and improving the documentation to make it more of a "first class" boost module.
It would also be interesting to investigate if there is some sort of lock free solution - At least on some compilers/ processors.
I'm not going to touch lock-free lazily initialized singletons! I'd rather have my code work ;-)
Just goes to show that no good idea goes unpunished.
:-)
SOOO I'm am positive about the idea. However, like most good ideas, its a lot bigger than it first appears.
Don't see why myself. In multi-threaded builds of serialization, we link with Boost.Thread and use boost::once.
Having written this, it would seem that this is territory well traveled by those working on a good Singleton suitable for such a purpose.
I've worked on the above singleton, tested it on a few x86 OSes and it seems to work. But I would be very happy if someone has a better solution that is tested.
(Amazingly, I don't even know if boost has such a thiing) Ideally, I would just like to use that (assuming it doesn't import everything on planet earth).
I believe that Boost has everything you need for a *correct* solution.
Sohail
Thanks for your quick response. Could you point me to the files that have been changed to address the issues?
You'll have to browse the CVS - look into extended type info
I am looking at:
http://boost.cvs.sourceforge.net/boost/boost/libs/serialization/src/extended_type_info.cpp?r1=1.11&r2=1.12 which I assume is the change you are suggesting.
I have tried this variant and it doesn't work with multiple-registred classes (from diferent DLLs) even in single-threaded environment. multiset is not enough because comparison of different instances of extended_type_info of the same double-registred class still returns false. I have changed my local copy of serialization library to properly handle multiregistration, however my solution is not threadsafe in general - it just works in one particular case. If someone interested in it, I can send the patch against 1.34 version.
Even though they happen to intersect at the same spot - There are two separate issues. a) The need for a thread-safe singleton b) the properway to handle multiple registrations for dynamically loaded code. I thought I addressed the second. (Now you've made me doubt that). The first is still an open issue. Robert Ramey Sergey Skorniakov wrote:
Thanks for your quick response. Could you point me to the files that have been changed to address the issues?
You'll have to browse the CVS - look into extended type info
I am looking at:
http://boost.cvs.sourceforge.net/boost/boost/libs/serialization/src/extended_type_info.cpp?r1=1.11&r2=1.12 which I assume is the change you are suggesting.
I have tried this variant and it doesn't work with multiple-registred classes (from diferent DLLs) even in single-threaded environment. multiset is not enough because comparison of different instances of extended_type_info of the same double-registred class still returns false. I have changed my local copy of serialization library to properly handle multiregistration, however my solution is not threadsafe in general - it just works in one particular case. If someone interested in it, I can send the patch against 1.34 version.
Particulary, I had problems with unregistered base-derived casts when tried to use multiset implemetation of tkmap and base or derived was registered from more than one dll. I had attached modified files that resolves it. The main issue is comparison of void_casters and basic_serializers, but there are also a couple of some less important changes like purging of void_caster_registry and correctness of basic_archive_impl::helper_compare.
Also, there are one still unresolved issue with multiple registration - sometimes loading failed if type serialized by value in one exe/dll and by pointer in another. I had not discovered this situation completely, but found that this occurs because loading code tries to read tracking info that absent in file (or the contrary). Fortunately, it is easy to write never-called-function that "loads" problematic type by pointer and place it to dll. But some kind of assert to recognize such a situation would be very usefull.
"Robert Ramey"
Even though they happen to intersect at the same spot - There are two separate issues.
a) The need for a thread-safe singleton b) the properway to handle multiple registrations for dynamically loaded code.
I thought I addressed the second. (Now you've made me doubt that). The first is still an open issue.
Robert Ramey
Sergey Skorniakov wrote:
Thanks for your quick response. Could you point me to the files that have been changed to address the issues?
You'll have to browse the CVS - look into extended type info
I am looking at:
http://boost.cvs.sourceforge.net/boost/boost/libs/serialization/src/extended_type_info.cpp?r1=1.11&r2=1.12 which I assume is the change you are suggesting.
I have tried this variant and it doesn't work with multiple-registred classes (from diferent DLLs) even in single-threaded environment. multiset is not enough because comparison of different instances of extended_type_info of the same double-registred class still returns false. I have changed my local copy of serialization library to properly handle multiregistration, however my solution is not threadsafe in general - it just works in one particular case. If someone interested in it, I can send the patch against 1.34 version.
-----Original Message----- From: Sohail Somani Sent: Monday, May 21, 2007 7:43 PM To: 'boost-users@lists.boost.org' Subject: RE: [Boost-users] [Serialization] Thread-unsafe singleton
Thanks for your quick response. Could you point me to the files that have been changed to address the issues?
In any case, I've been able to reproduce it pretty easily on my side and am working on disentangling the test from my project. Would you be interested if I can extract a test case?
Don't hold your breath for that test case. Obvious attempt 0 has failed! Sohail
participants (3)
-
Robert Ramey -
Sergey Skorniakov -
Sohail Somani