Created attachment 72874 [details] [review]
Proposed patch for evolution-data-server

This patch converts all uses of EMutex to either GStaticMutex or GStaticRecMutex, and also removes EMutex from e-msgport.[ch].  Camel's public API is unchanged.

I made every effort to minimize changes to the code, though I did have to define new LOCK/UNLOCK macros for a few of Camel's private data structures that use both recursive and non-recursive mutexes.

Created attachment 72875 [details] [review]
Proposed patch for evolution

There is a single occurrence of EMutex in Evolution.  It's created and destroyed with MailSession instances, but never actually used.  So I removed it.

Note that the last two hunks of attachment #72874 [details] -- which remove EMutex from libedataserver -- can be snipped if we want to just deprecate EMutex.

Hooray! Excellent stuff Matthew

Little sidenote:

camel-imap4-folder.c: In function 'imap4_sync':
camel-imap4-folder.c:555: error: 'GStaticRecMutex' has no member named 'static_mutex'
camel-imap4-folder.c:555: error: 'GStaticRecMutex' has no member named 'runtime_mutex'
camel-imap4-folder.c:555: error: 'GStaticRecMutex' has no member named 'runtime_mutex'
camel-imap4-folder.c:555: error: 'GStaticRecMutex' has no member named 'runtime_mutex'
camel-imap4-folder.c:623: error: 'GStaticRecMutex' has no member named 'static_mutex'
camel-imap4-folder.c:623: error: 'GStaticRecMutex' has no member named 'runtime_mutex'
camel-imap4-folder.c:623: error: 'GStaticRecMutex' has no member named 'runtime_mutex'
camel-imap4-folder.c:623: error: 'GStaticRecMutex' has no member named 'runtime_mutex'
camel-imap4-folder.c: In function 'imap4_refresh_info':
camel-imap4-folder.c:644: error: 'GStaticRecMutex' has no member named 'static_mutex'
camel-imap4-folder.c:644: error: 'GStaticRecMutex' has no member named 'runtime_mutex'

This is A Very Useful Thing to Have. Bumping up priority.

Created attachment 74969 [details] [review]
Revised patch for evolution-data-server

Thanks Philip, I forgot to compile with --enable-imap4.

Here's a revised patch that fixes those errors, with updated ChangeLogs.

Wasn't this commited already?

Not yet.  CVS HEAD still has Camel using EMutex all over the place.

GMutex existed before EMutex so I'm not confidant in these patches... I don't remember the reason we wrote EMutex and so before committing these patches, I'd really like to know what the purpose of EMutex was. Perhaps an email to NotZed would shed some light? (Not that I really want to disturb him for Evo discussion, since if I wasn't working for Novell anymore, I probably wouldn't want to be bothered).

Based on comments in the E-D-S source code and CVS archives, GLib had GMutex but no recursive mutex at the time EMutex was written.  Now that it does, I'm simply proposing to replace all instances of EMutex with GStaticMutex or GStaticRecMutex as appropriate.

ah, right... ok, then I'm happy :)

Created attachment 77742 [details] [review]
Revised patch for evolution-data-server

Here's a revised patch for evolution-data-server-1.9.3.  It adapts to some new logic in the GroupWise CamelProvider.

Also in this revision, instead of removing the EMutex code I tagged it as deprecated by wrapping the relevant declarations in a preprocessor conditional like so:

   #ifndef EDS_DISABLE_DEPRECATED

   /* ... EMutex declarations ... */

   #endif

This is similar to what GTK+ does for deprecated symbols.

varadhan has concerns about removing e_mutex_assert_locked(), which asserts that the current thread owns the recursive EMutex.  Several CamelProvider subclasses use this assertion for debugging purposes.  The issue is that GLib currently offers no (documented) way to make a similar assertion with a GStaticRecMutex.

Bug #387385 proposes a new GStaticRecMutex function which will allow for such an assertion.  But this will not be available until GLib 2.14 at the earliest, so I proposed that in the meantime we kludge the needed functionality by accessing undocumented components of GStaticRecMutex and the GThread module.

If accepted, we should be able to use the new function in e-d-s-1.11/1.12.

It looks like the GLib bug I proposed was not as well received as I had hoped, and probably for good reason now that I've had time to reflect on it more.

So that leaves a few options:

  1) Deprecate EMutex, migrate Camel et. al. to the mutex implementations
     now provided by GLib and live without the ability to assert that the
     current thread has locked the mutex.

  2) Deprecate EMutex, migrate Camel et. al. to the mutex implementations
     now provided by GLib and implement the lock assertion by hacking on
     undocumented parts of GStaticRecMutex.

  3) Gut EMutex such that it becomes a wrapper for the mutex implementations
     now provided by GLib without breaking API.  Keep e_mutex_assert_locked()
     functional by hacking on undocumented parts of GStaticRecMutex.

  4) Mark this bug as WONTFIX.

I'm still not entirely sure whether options 2 and 3 are possible.

I favor option 1 for reasons stated in bug #387385.

varadhan: ping - Any thoughts on this?

I favor Part-1 of 3 - Get EMutex such that it becomes a wrapper for mutex implementations now provided by GLib without breaking API.  This way, it is easy to move to different mutex implementations, without changing much of the code.

I am okay to deprecate e_mutext_assert_locked().   

I was attempting an implementation of EMutex as a wrapper for the GLib implementations and hit another snag.

   /* this uses pthread cond's */
   e_mutex_cond_wait(void *cond, EMutex *m);

I don't know why the "cond" argument is a void pointer.  The implementation converts it to a pthread_cond_t pointer.  The problem is GLib does not take pthread_cond_t for its "mutex-wait" functions.  It implements/wraps its own condition variable (GCond), which is not necessarily compatible with pthread_cond_t (e.g. in a Windows environment).

    From gthread-win32.c:

    struct _GCond
    {
      GPtrArray *array;
      CRITICAL_SECTION lock;
    };

I don't know how to work around this.  Having the function take a GCond would break EMutex's API and defeat the purpose of converting EMutex to a GLib mutex wrapper.  But I can't reliably make pthread_cond_t work with the GLib mutex implementations.

It's too bad the original implementor didn't think to provide an ECond struct.

varadhan: How do you want to proceed?

varadhan and I discussed this at length on IRC and we finally agreed go with the original proposal of deprecating EMutex and migrating to the mutexes provided by GLib (GStaticMutex and GStaticRecMutex).  This primarily affects Camel, but does not change its public API or ABI.

I will commit the patches in comment #2 and comment #13.

Committed both patches to Subversion trunk.