Attached is a patch which changes Mutex to handle the initialization
ordering problem where Lock can be called before the constructor
is called.
On Wed, Apr 15, 2009 at 1:41 PM, Wink Saville <w...@google.com> wrote:
> Fair enough on the Mutex, I'll try to get a new patch to you soon,
> but if you get there first, great.
>
> As for the explicit initialization I humbly disagree but I understand
> your point completely. Maybe someday there will be another
> initialization issue that isn't as easily solved and it can be revisited.
>
> -- Wink
>
>
>
> On Wed, Apr 15, 2009 at 12:25 PM, Kenton Varda <ken...@google.com> wrote:
>
>> On Tue, Apr 14, 2009 at 11:07 AM, Wink Saville <w...@google.com> wrote:
>>
>>> Feels strange to be using the object before it's constructed. Also, every
>>> Mutex has the cost of the test.
>>
>>
>> A predictable branch is basically free, especially compared to the cost of
>> the atomic ops needed to actually lock a mutex.
>>
>>
>>> If you want to go this way what about
>>> a "StaticMutex" which was implemented this way and leave the Mutex
>>> alone.
>>
>>
>> The goal here is to have the same behavior as the Mutex class in Google's
>> internal codebase, so having a separate class doesn't seem like a good idea.
>>
>>
>>> I'm still in favor of an explicit initialization as a long term solution
>>> where
>>> all ordering issues are controlled. Your thoughts on this?
>>
>>
>> To me it sounds like a lot of work without very much benefit.
>>
>>
>>>
>>>
>>>
>>> On Tue, Apr 14, 2009 at 10:29 AM, Kenton Varda <ken...@google.com>wrote:
>>>
>>>> I think you could just do something like:
>>>> enum LinkerInitialized { LINKER_INITIALIZED };
>>>>
>>>> // Mutex allocated in static memory which may be locked before the
>>>> // constructor is actually called. We can take advantage of the fact
>>>> // that static memory is always initialized to zero.
>>>> Mutex::Mutex(LinkerInitialized) {
>>>> if (impl_ == NULL) {
>>>> Init();
>>>> }
>>>> }
>>>>
>>>> Mutex::Lock() {
>>>> if (impl_ == NULL) {
>>>> // We are a linker-initialized mutex and we're being locked
>>>> // before the constructor has been called. There should be
>>>> // no threads running yet, so we can just initialize now.
>>>> Init();
>>>> }
>>>> }
>>>>
>>>> On Mon, Apr 13, 2009 at 10:12 PM, Wink Saville <w...@google.com> wrote:
>>>>
>>>>> I did some searching and my initial reaction is that this requires some
>>>>> initialization
>>>>> of the mutex at load time which may not be possible in windows. But if
>>>>> you have
>>>>> a technique in mind let me know.
>>>>>
>>>>> I wonder if another option might be better in the long term. And that
>>>>> is to have an
>>>>> initialization class that is statically allocated which does all the
>>>>> initialization.
>>>>> This would allow the initialization order to be well defined within the
>>>>> protobuf
>>>>> module. Also this would make protobuf more amenable to embedded systems
>>>>> where reloads my not occur and an explicit initialization call would be
>>>>> needed.
>>>>>
>>>>> I'd be glad to do a first pass at coding this if people would be
>>>>> interested.
>>>>>
>>>>> -- Wink
>>>>>
>>>>>
>>>>> On Mon, Apr 13, 2009 at 5:26 PM, Kenton Varda <ken...@google.com>wrote:
>>>>>
>>>>>> BTW, probably the ideal approach would be to make the Mutex class
>>>>>> itself immune to initialization ordering problems. That is, if you
>>>>>> allocate
>>>>>> a Mutex statically, it shouldn't matter if its constructor has been run
>>>>>> yet
>>>>>> when you try to lock it. (The Mutex class in our internal codebase works
>>>>>> like this but is non-portable, hence the different implementation in the
>>>>>> open source release.) If you want to update your patch, go ahead,
>>>>>> otherwise
>>>>>> I'll take care of implementing this when it gets to the top of the queue.
>>>>>> :)
>>>>>>
>>>>>> On Mon, Apr 13, 2009 at 5:21 PM, Kenton Varda <ken...@google.com>wrote:
>>>>>>
>>>>>>> My backlog of patches and other work is very long, unfortunately.
>>>>>>> But I hope to spend some time reducing it this week, hopefully getting
>>>>>>> to
>>>>>>> this.
>>>>>>>
>>>>>>>
>>>>>>> On Sat, Apr 11, 2009 at 12:13 PM, Wink Saville <w...@google.com>wrote:
>>>>>>>
>>>>>>>> Has anyone been able to look at this, do I need to make some
>>>>>>>> changes or do anything else?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Apr 8, 2009 at 2:59 PM, Kenton Varda <ken...@google.com>wrote:
>>>>>>>>
>>>>>>>>> Although I see the decsriptor.cc part isn't meant to be
>>>>>>>>> submitted...
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Apr 8, 2009 at 2:59 PM, Kenton Varda <ken...@google.com>wrote:
>>>>>>>>>
>>>>>>>>>> You can use "svn diff" to produce a single patch file that
>>>>>>>>>> contains both parts of this change.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Apr 7, 2009 at 10:01 PM, Wink Saville <w...@google.com>wrote:
>>>>>>>>>>
>>>>>>>>>>> There is a bug in the initialization of the
>>>>>>>>>>> log_silencer_count_mutex_ in common.cc.
>>>>>>>>>>> Currently it is initialized because it is a global object, but
>>>>>>>>>>> the order of initialization
>>>>>>>>>>> of global objects is undefined in c++ and in my environment it
>>>>>>>>>>> was not initialized
>>>>>>>>>>> when some errors were encountered as I was debugging.
>>>>>>>>>>>
>>>>>>>>>>> If descriptor.cc.patch is applied without applying
>>>>>>>>>>> common.cc.patch the compilation
>>>>>>>>>>> fails when the GOOGLE_LOG(ERROR) << "IBGF"; executes because the
>>>>>>>>>>> log_silencer_count_mutex_ wasn't yet initialized. It so happens
>>>>>>>>>>> that this actually
>>>>>>>>>>> fails at build time because the descriptor code is executed
>>>>>>>>>>> during compiliation.
>>>>>>>>>>>
>>>>>>>>>>> If you then apply common.cc.patch the compile completes
>>>>>>>>>>> successfully and make
>>>>>>>>>>> check also succeeds.
>>>>>>>>>>>
>>>>>>>>>>> The patch initializes the mutex on first use rather than at
>>>>>>>>>>> initialization time thus does not
>>>>>>>>>>> suffer the problem of the current scheme. It does make me wonder
>>>>>>>>>>> if there
>>>>>>>>>>> maybe other latent bugs in the code associated with assumed
>>>>>>>>>>> initialization order.
>>>>>>>>>>>
>>>>>>>>>>> Please review this patch and if additional changes are desired
>>>>>>>>>>> I'll be glad
>>>>>>>>>>> to work through any changes.
>>>>>>>>>>>
>>>>>>>>>>> -- Wink
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> >>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Protocol Buffers" group.
To post to this group, send email to protobuf@googlegroups.com
To unsubscribe from this group, send email to
protobuf+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/protobuf?hl=en
-~----------~----~----~----~------~----~------~--~---
Index: src/google/protobuf/stubs/common.cc
===================================================================
--- src/google/protobuf/stubs/common.cc (revision 105)
+++ src/google/protobuf/stubs/common.cc (working copy)
@@ -113,7 +113,7 @@
static LogHandler* log_handler_ = &DefaultLogHandler;
static int log_silencer_count_ = 0;
-static Mutex log_silencer_count_mutex_;
+static Mutex log_silencer_count_mutex_(Mutex::LINKER_INITIALIZED);
static string SimpleCtoa(char c) { return string(1, c); }
@@ -197,6 +197,10 @@
#ifdef _WIN32
struct Mutex::Internal {
+ Internal() {
+ InitializeCriticalSection(&mutex);
+ }
+
CRITICAL_SECTION mutex;
#ifndef NDEBUG
// Used only to implement AssertHeld().
@@ -204,17 +208,25 @@
#endif
};
-Mutex::Mutex()
- : mInternal(new Internal) {
- InitializeCriticalSection(&mInternal->mutex);
+Mutex::Mutex() {
+ mInternal = new Internal();
}
+Mutex::Mutex(LinkerInitialized) {
+ if (mInternal == NULL) {
+ mInternal = new Internal();
+ }
+}
+
Mutex::~Mutex() {
DeleteCriticalSection(&mInternal->mutex);
delete mInternal;
}
void Mutex::Lock() {
+ if (mInternal == NULL) {
+ mInternal = new Internal();
+ }
EnterCriticalSection(&mInternal->mutex);
#ifndef NDEBUG
mInternal->thread_id = GetCurrentThreadId();
@@ -237,20 +249,33 @@
#elif defined(HAVE_PTHREAD)
struct Mutex::Internal {
+ Internal() {
+ pthread_mutex_init(&mutex, NULL);
+ }
+
pthread_mutex_t mutex;
};
-Mutex::Mutex()
- : mInternal(new Internal) {
- pthread_mutex_init(&mInternal->mutex, NULL);
+
+Mutex::Mutex() {
+ mInternal = new Internal();
}
+Mutex::Mutex(LinkerInitialized) {
+ if (mInternal == NULL) {
+ mInternal = new Internal();
+ }
+}
+
Mutex::~Mutex() {
pthread_mutex_destroy(&mInternal->mutex);
delete mInternal;
}
void Mutex::Lock() {
+ if (mInternal == NULL) {
+ mInternal = new Internal();
+ }
int result = pthread_mutex_lock(&mInternal->mutex);
if (result != 0) {
GOOGLE_LOG(FATAL) << "pthread_mutex_lock: " << strerror(result);
Index: src/google/protobuf/stubs/common.h
===================================================================
--- src/google/protobuf/stubs/common.h (revision 105)
+++ src/google/protobuf/stubs/common.h (working copy)
@@ -1006,13 +1006,29 @@
// while holding it, T will deadlock.
class LIBPROTOBUF_EXPORT Mutex {
public:
+ enum LinkerInitialized { LINKER_INITIALIZED };
+
// Create a Mutex that is not held by anybody.
Mutex();
+ // Create a Mutex initialized at link time.
+ //
+ // This constructor maybe called after the first call to Lock because in
+ // C++ there is no guarantee of initialization ordering across compiliation
+ // units. Thus the Lock routine maybe called before this constructor. The
+ // typical solution is to know that mInteral will be NULL and to initialize
+ // mInternal in this contrustor and in Lock if it is NULL. Note it is not
+ // necessary to have any other synchronization mechanism because startup
+ // initialization is not multi-threaded.
+ Mutex(LinkerInitialized);
+
// Destructor
~Mutex();
// Block if necessary until this Mutex is free, then acquire it exclusively.
+ //
+ // As explained above the implemenation must handle the situation where this
+ // is called before the constructor.
void Lock();
// Release this Mutex. Caller must hold it exclusively.
