Error handling in win32 InterProcessLock

I’m not sure InterProcessLock::enter always does the right thing if one of the underlying OS calls fails.

For example, if CreateMutex indicates ERROR_ALREADY_EXISTS but timeoutMillisecs is 0, InterProcessLock::enter calls ReleaseMutex but the process doesn’t own the mutex at that point. Plus also, reentrancyLevel is 1 so any further calls to InterProcessLock::enter don’t try to create the mutex again.

It’s also possible for WaitForSingleObject to return other interesting things besides WAIT_TIMEOUT. Here’s my current version of the function.[code]bool InterProcessLock::enter (const int timeOutMillisecs)
{
String msg;

if (reentrancyLevel++ == 0)
{
    // http://msdn.microsoft.com/en-us/library/ms682411%28VS.85%29.aspx
    // says that if the name starts with
    // Global that it can't contain any backslash (\)
    // characters.
    internal = CreateMutex (0, TRUE, "Global\\" +
                            name.replaceCharacter('\\','/'));

    if (internal == 0)
    {
        msg << __FUNCTION__ << ": CreateMutex(" << name << ") failed (" << GetLastError() << ")";
        Logger::writeToLog(msg);
        return false;
    }

    if (GetLastError() == ERROR_ALREADY_EXISTS)
    {
        if (timeOutMillisecs == 0)
        {
            // Another process has the lock and we were
            // told not to wait, so give up
            CloseHandle (internal);
            internal = 0;
            reentrancyLevel--;
            return false;
        }

        DWORD retval;

        retval = WaitForSingleObject (internal, (timeOutMillisecs < 0) ? INFINITE : timeOutMillisecs);
        switch (retval) {
        case WAIT_ABANDONED:
            msg << __FUNCTION__ << ": another thread abandoned \"" << name << "\"";
            Logger::writeToLog(msg);
            // We've got the lock, so return success
            return true;
        case WAIT_OBJECT_0:
            // We got the lock before the timeout expired
            return true;
        case WAIT_TIMEOUT:
            // We didn't get the lock
            CloseHandle (internal);
            internal = 0;
            reentrancyLevel--;
            return false;
        default:
            // Unknown return value from WaitForSingleObject
            msg << __FUNCTION__ << ": WaitForSingleObject(" << name << ") returned unknown value " << retval;
            Logger::writeToLog(msg);
            reentrancyLevel--;
            return false;
            
        }
    }
}

return (internal != 0);

}[/code]Plus also, I’m not sure the InterProcessLock destructor does the right thing. If the goal is to make sure the mutex is released before the object goes away, shouldn’t it handle the case where reentrancyLevel is > 1, either by calling exit multiple times or with its own code…maybe like this:[code]InterProcessLock::~InterProcessLock()
{
if ((reentrancyLevel > 0) && (internal != 0))
{
ReleaseMutex (internal);
}

if (internal != 0)
{
    CloseHandle (internal);
}

}[/code]And finally, I’m nervous about reentrancyLevel becoming less than 0 if someone calls exit repeatedly. I suppose it could be by design but if only to match what the posix code does, shouldn’t InterProcessLock::exit start with if (reentrancyLevel > 0 && internal != 0) { --reentrancyLevel;-DB

P.S. To be really paranoid we could check the return value of ReleaseMutex in InterProcessLock::exit and change the signature to return a bool.

Ok, thanks - that’s pretty old code in there, I’ll take a look and make sure it’s watertight!

I don’t understand the msdn doc like you did. For me, CreateMutex sets ERROR_ALREADY_EXISTS it doesn’t mean you’ve failed entering the mutex (unless you’re trying to acquire at the same time).
It simply says that some other process opened the mutex with this name (but it doesn’t say anything about the mutex’s state).

To me your code is correct only if you only intend to deal with other Juce process.
As soon as another process (not Juce based) opens the mutex, and keep it open, the code will likely don’t work anymore.
It would probably be better if InterprocessLock created the mutex in it’s constructor (but without InitialOwner), and then deal with way simpler code in enter / exit (which is where you want SPEED) with simple “WaitForSingleObject” and “ReleaseMutex” there.

To avoid reentrancy issue, it’s better to use CreateSemaphore with a count set to 1.

Don’t know if you guys saw it, but I reimplemented the class yesterday, using a pimpl pattern to tidy it up.

The docs do say that WAIT_ABANDONED means that the caller thread gained the mutex, so I think I’ve done that correctly…

Sorry, I didn’t read the code.
Pimpl like loki’s version or pimpl with the “impl *” member that requires an additional ptr dereference for every operation ?

Not quite sure what you mean… I’ve never seen Loki’s version, but any pimpl implementation must use a pointer, so will inevitably need to dereference it somehow. I generally use a ScopedPointer, and unless I’m being thick, there’s no way you could do it more efficiently (?)

It’s possible to use PImpl using stack storage. Loki used to do this (but I haven’t checked if it’s still the case). It avoids dereference on access, provided compiler inline stuff correctly.
The idea is that you’ll declare an object like this:

template <typename Policy> // Policy is either ObjectCopyPolicy (Value) or ObjectPointerPolicy (Pointer) 
class ImplT
{
    public:
	    /** Is the implementation empty ? */
	    struct Empty {};

	    /** This is the maximum implementation size before it is stored automatically in the heap */
	    enum { MaximumStaticSize = sizeof(long double) };

    private:

	    /** The union itself (to allow the static & dynamic behaviour) */
	    union InternalUnion
	    {
		    uint8	Buffer[MaximumStaticSize];
		    void *  Pointer;

		    /** This is not truly portable, we should ensure the alignment with a type traits in a ideal case */
		    long double used_for_alignment;
		    long used_for_alignmentInOtherCase;
	    };
	    
	    /** The operation on the unions */
	    struct VirtualTable 
	    {
		    void*									(*getPointer)(InternalUnion&);
		    const void*								(*getConstPointer)(const InternalUnion&);
		    void									(*Destructor)(InternalUnion&);
		    void									(*Deleter)(InternalUnion&);
		    void									(*Clone)(InternalUnion&, const InternalUnion&);
	    };
	    /** Specialization for stack based storage */
	    template <typename T> 
	    struct VirtualTableBigImpl
	    {
		    template <typename U> struct Inner;
		    template <> struct Inner< Bool2Type<true> >
		    {
			    static void* getPointer(InternalUnion& x)					{ return x.Buffer; } 
			    static const void* getConstPointer(const InternalUnion& x)	{ return x.Buffer; } 
			    static void  Destructor(InternalUnion& x)					{ Cast(x)->~T();  }
			    static void  Deleter(InternalUnion& x)						{ Destructor(x); x.Pointer = NULL; }
			    static void  Clone(InternalUnion& x, const InternalUnion& y){ new(x.Buffer) T(*Cast(y)); }

			    static T* Cast(InternalUnion& x)							{ return reinterpret_cast<T*>(x.Buffer); }
			    static const T* Cast(const InternalUnion& x)				{ return reinterpret_cast<const T*>(x.Buffer); }
		    };

		    template <> struct Inner< Bool2Type<false> >
		    {
			    static void* getPointer(InternalUnion& x)					{ return x.Pointer; } 
			    static const void* getConstPointer(const InternalUnion& x)	{ return x.Pointer; } 
			    static void  Destructor(InternalUnion& x)					{ Cast(x)->~T();  }
			    static void  Deleter(InternalUnion& x)						{ delete(Cast(x)); }
			    static void  Clone(InternalUnion& x, const InternalUnion& y){ x.Pointer = new T(*Cast(y)); }

			    static T* Cast(InternalUnion& x)							{ return reinterpret_cast<T*>(x.Pointer); }
			    static const T* Cast(const InternalUnion& x)				{ return reinterpret_cast<const T*>(x.Pointer); }
		    };
	    };

	    template<typename T> 
	    static VirtualTable* getTable(T*)  
	    {
		    /** Selection is done here at compile time */
		    enum { Optimize = sizeof(T) <= MaximumStaticSize };

		    /** Okay, get the pointers */
		    static VirtualTable virtualTable = 
		    {
		      , &VirtualTableBigImpl<T>::Inner< Bool2Type<Optimize> >::getPointer
		      , &VirtualTableBigImpl<T>::Inner< Bool2Type<Optimize> >::getConstPointer
		      , &VirtualTableBigImpl<T>::Inner< Bool2Type<Optimize> >::Destructor
		      , &VirtualTableBigImpl<T>::Inner< Bool2Type<Optimize> >::Deleter
		      , &VirtualTableBigImpl<T>::Inner< Bool2Type<Optimize> >::Clone
		    };
		    return & virtualTable;
	    }	
	    // Interface
    public:
	    // Construction / Destruction
	    /** Default constructor */
	    ImplT() : table(getTable((Empty*)0)) { inner.Pointer = NULL; }
	    /** Value constructor */
	    template <typename T>
		    ImplT(const T & x) : table(getTable((Empty*)0)) { inner.Pointer = NULL; Init(x); }
	    /** Value constructor */
	    template <typename T>
		    ImplT(int, const T & x) : table(getTable((Empty*)0)) { inner.Pointer = NULL; InitDefault(x); }
	    /** Copy constructor */
	    ImplT(const ImplT & x) : table(getTable((Empty*)0)) { inner.Pointer = NULL; Set(x); }

	    /** Default destructor */
	    ~ImplT()  { Reset();	}    

	    // Helpers methods 
    public:
	    /** Initialization to a specified type at runtime */
	    template<typename T>
	    void Init(const T& x) 
	    {
		    table = getTable((T*)0);
		    if (Policy::makeCopy)
		    {
			    if (sizeof(T) <= MaximumStaticSize)	new(inner.Buffer) T(x);
			    else 								inner.Pointer = new T(x); 
		    }
		    else inner.Pointer = reinterpret_cast<void*>(const_cast<T*>(&x));
	    }
	    /** Initialization to a specified type at runtime using default constructor */
	    template<typename T>
	    void InitDefault(const T& x) 
	    {
		    table = getTable((T*)0);
		    if (Policy::makeCopy)
		    {
			    if (sizeof(T) <= MaximumStaticSize)	new(inner.Buffer) T();
			    else 								inner.Pointer = new T(); 
		    }
		    else inner.Pointer = reinterpret_cast<void*>(const_cast<T*>(&x));
	    }

	    ImplT& Set(const ImplT & x) 
	    {
		    Reset();				table = x.table;	  
		    if (Policy::makeCopy)	table->Clone(inner, x.inner);
		    else 					inner.Pointer = x.inner.Pointer;
		    return *this;
	    }

	    template<typename T>
	    inline ImplT& operator=(const T& x)			{ return Set(ImplT(x)); }

	    inline ImplT& operator=(const ImplT& x)	{ return Set(x);	}
	    template<typename T>
	    inline T* toPointer(T*) 
	    {	if (Policy::makeCopy)	return reinterpret_cast<T*>(table->getPointer(inner));
		    else 					return reinterpret_cast<T*>(inner.Pointer);	}

	    template<typename T>
	    inline T& direct(T*) 
	    {	return *reinterpret_cast<T*>(table->getPointer(inner)); }

	    template<typename T>
	    inline const T* toPointer(const T *) const 
	    {	if (Policy::makeCopy)	return reinterpret_cast<const T*>(table->getConstPointer(inner));
		    else					return reinterpret_cast<const T*>(inner.Pointer);	}

	    template<typename T>
	    inline void toRef(T& t) 
	    {	if (Policy::makeCopy)	t = *reinterpret_cast<T*>(table->getPointer(inner));
		    else 					t = *reinterpret_cast<T*>(inner.Pointer);	}

	    template<typename T>
	    inline void toRef(const T & t) const 
	    {	if (Policy::makeCopy)	*const_cast<T*>(&t) = *reinterpret_cast<const T*>(table->getConstPointer(inner));
		    else					*const_cast<T*>(&t) = *reinterpret_cast<const T*>(inner.Pointer);	}


	    /** Check if this variant is currently empty */
	    inline bool isEmpty() const { return table == getTable((Empty*)0); }

	    /** Reset the variant */
	    inline void Reset() 
	    {
		    if (isEmpty()) return; 
		    if (Policy::makeCopy)	table->Deleter(inner);
		    table = getTable((Empty*)0);
	    }

	    /** Container */
    private:
	    /** The type to object mapper functions */
	    VirtualTable * table;
	    /** The internal union */
	    InternalUnion inner;
};

    struct ObjectPtrPolicy 
    {
	    enum { makeCopy = false }; 
    };

    struct ObjectCopyPolicy 
    {
	    enum { makeCopy = true }; 
    };
typedef ImplT<ObjectCopyPolicy> Impl;

type

Then usage is quite simple:

// In .h
#include "Impl.h"
// Don't want to know the implementation called Bob
class Alice
{
    // Interface
    int getPhoneNumber();

    Alice();
private:
    Impl impl;
}

// In .cpp
#include "Alice.h"
struct Bob
{
    int phoneNumber;
    Bob(int val) : phoneNumber(val) {}
};

int Alice::getPhoneNumber() { return impl.direct((Bob*)0).phoneNumber; }

Alice::Alice() : impl(Bob(2345)) {}

Impl decides at compile time if it’s better to store the value on the stack or on the heap (you can adjust the limit in the code). It’s type safe.
A smart compiler should remove the *(&Buffer) to a direct Buffer usage.

Blimey. The words “sledgehammer” and “nut” spring to mind!

It’s a nice example of cunning C++ coding, but I find it hard to think of a situation which would justify all that extra faffing about. Pimpl classes don’t tend to be used in tight inner loops that require maximum performance, and that’s a lot of code to (possibly) save a few nanoseconds per method call!

[quote=“jules”]Don’t know if you guys saw it, but I reimplemented the class yesterday, using a pimpl pattern to tidy it up.

The docs do say that WAIT_ABANDONED means that the caller thread gained the mutex, so I think I’ve done that correctly…[/quote]

I think you got it right, but I’m a little confused about why refCount is a member of InterProcessLock::Pimpl and not InterProcessLock. The only place it’s used is in the InterProcessLock::Pimpl constructor. Does InterProcessLock::Pimpl::close need to use it? At least a little related, would it be safer for InterProcessLock::exit to make sure refCount never goes below 0…at least with an assert?

The refcount stuff looks fine to me… Good point about adding an assertion though - I’ll do that.