Re: [evolution-patches] fix for bug #61551

From: Not Zed <notzed ximian com>
To: Jeffrey Stedfast <fejj ximian com>
Cc: evolution-patches ximian com
Subject: Re: [evolution-patches] fix for bug #61551
Date: Fri, 16 Jul 2004 11:44:21 +0800

On Thu, 2004-07-15 at 13:32 -0400, Jeffrey Stedfast wrote:

On Thu, 2004-07-15 at 15:12 +0800, Not Zed wrote:
> 
> This doesn't look good, shouldn't be poking the other folders
> internals, more than it already is anyway.
> 
> I think it would be better to just not call camel_folder_refresh_info
> with the connect lock held.  Infact since we're only comparing the
> current folder which is the only lockable item, it should be safe to
> remove the connect locks enitrely.  refresh_info will lock in the
> right order.

ok... if we remove the connect_lock, isn't there a race checking dest !=
current?

or maybe that just doesn't matter...?

No, becuase we don't actually dereference the pointer.

What could happen?

we lock
we read the pointer (atomic operaiton)
we update
we unlock

or with no locking
we read the pointer
we go to start updating
the pointer changes
we keep updating, getting the folder/connect locks in the process
we unlock

the updating process will set the current folder back anyway.

and the other option

we read the pointer, it is the same, so we do nothing
then another thread locks and changes the pointer
-> effectively no difference.

the summary count lookup is locked anyway
and the folder_refresh_info is locked too.

the connect lock is only protecting the pointer read, which is atomic anyway.

I've attached a patch with the removed connect_locks but I'd like to
know the answer to my above concern before I'd be willing to commit it.

Jeff

> 
> On Wed, 2004-07-14 at 15:38 -0400, Jeffrey Stedfast wrote: 
> > http://bugzilla.ximian.com/show_bug.cgi?id=61551
> > 
> > deadlock condition where one thread has the connect_lock and tries to
> > get the folder_lock and another thread which already has the folder_lock
> > tries to get the connect_lock.
> > 
> > Jeff
> > 
> > Plain text document attachment (61551.patch)
> > ? 55280.patch
> > ? 61538.patch
> > ? 61551.patch
> > ? ChangeLog.nonximian
> > ? body
> > ? body.c
> > ? body.txt
> > ? body2.txt
> > ? braindamaged.patch
> > ? camel-namespace.patch
> > ? cf.c
> > ? charset-map.c
> > ? charset-map.diff
> > ? class.sh
> > ? cmsutil.c
> > ? date.patch
> > ? flags
> > ? foo
> > ? foo.txt
> > ? foo2.txt
> > ? gw-body.txt
> > ? imap
> > ? invalid-content-id.patch
> > ? iso
> > ? iso.c
> > ? namespace.sh
> > ? patch
> > ? pop3-uidl.patch
> > ? smime
> > ? uid-cache.patch
> > ? providers/tmp
> > ? providers/local/camel-mozilla-folder.c
> > ? providers/local/camel-mozilla-folder.h
> > ? providers/local/camel-mozilla-store.c
> > ? providers/local/camel-mozilla-store.h
> > ? tests/data/camel-mime-tests.tar.gz
> > Index: ChangeLog
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
> > retrieving revision 1.2219
> > diff -u -r1.2219 ChangeLog
> > --- ChangeLog	14 Jul 2004 19:00:37 -0000	1.2219
> > +++ ChangeLog	14 Jul 2004 19:25:16 -0000
> > @@ -1,5 +1,11 @@
> >  2004-07-14  Jeffrey Stedfast  <fejj novell com>
> >  
> > +	* providers/imap/camel-imap-folder.c (imap_transfer_online): Grab
> > +	the folder lock before grabbing the connect_lock when refreshing
> > +	the dest folder's info to avoid the deadlock in bug #61551.
> > +
> > +2004-07-14  Jeffrey Stedfast  <fejj novell com>
> > +
> >  	Fix for bug #61538
> >  
> >  	* camel-process.c (camel_process_fork): Same.
> > Index: providers/imap/camel-imap-folder.c
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/camel/providers/imap/camel-imap-folder.c,v
> > retrieving revision 1.336
> > diff -u -r1.336 camel-imap-folder.c
> > --- providers/imap/camel-imap-folder.c	20 May 2004 19:16:53 -0000	1.336
> > +++ providers/imap/camel-imap-folder.c	14 Jul 2004 19:25:16 -0000
> > @@ -1481,11 +1481,13 @@
> >  		return;
> >  
> >  	/* Make the destination notice its new messages */
> > +	CAMEL_FOLDER_LOCK(dest, lock);
> >  	CAMEL_SERVICE_LOCK (store, connect_lock);
> >  	if (store->current_folder != dest ||
> >  	    camel_folder_summary_count (dest->summary) == count)
> >  		camel_folder_refresh_info (dest, ex);
> >  	CAMEL_SERVICE_UNLOCK (store, connect_lock);
> > +	CAMEL_FOLDER_UNLOCK(dest, lock);
> >  	
> >  	if (delete_originals) {
> >  		for (i = 0; i < uids->len; i++)
> -- 
> 
> Michael Zucchi <notzed ximian com>
> "born to die, live to work, it's
> all downhill from here"
> Novell's Evolution and Free
> Software Developer

Plain text document attachment (61551.patch)

? 55280.patch
? 61551.patch
? ChangeLog.nonximian
? body
? body.c
? body.txt
? body2.txt
? braindamaged.patch
? camel-namespace.patch
? cf.c
? charset-map.c
? charset-map.diff
? class.sh
? cmsutil.c
? date.patch
? flags
? foo
? foo.txt
? foo2.txt
? gw-body.txt
? imap
? invalid-content-id.patch
? iso
? iso.c
? namespace.sh
? patch
? pop3-uidl.patch
? smime
? uid-cache.patch
? providers/tmp
? providers/local/camel-mozilla-folder.c
? providers/local/camel-mozilla-folder.h
? providers/local/camel-mozilla-store.c
? providers/local/camel-mozilla-store.h
? tests/data/camel-mime-tests.tar.gz
Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
retrieving revision 1.2219
diff -u -r1.2219 ChangeLog
--- ChangeLog	14 Jul 2004 19:00:37 -0000	1.2219
+++ ChangeLog	15 Jul 2004 17:36:48 -0000
@@ -1,3 +1,9 @@
+2004-07-15  Jeffrey Stedfast  <fejj novell com>
+
+	* providers/imap/camel-imap-folder.c (imap_transfer_online): Don't
+	grab the connect_lock before calling refresh_info so that we avoid
+	the deadlock in bug #61551.
+
 2004-07-14  Jeffrey Stedfast  <fejj novell com>
 
 	Fix for bug #61538
Index: providers/imap/camel-imap-folder.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/providers/imap/camel-imap-folder.c,v
retrieving revision 1.336
diff -u -r1.336 camel-imap-folder.c
--- providers/imap/camel-imap-folder.c	20 May 2004 19:16:53 -0000	1.336
+++ providers/imap/camel-imap-folder.c	15 Jul 2004 17:36:48 -0000
@@ -1481,11 +1481,9 @@
 		return;
 
 	/* Make the destination notice its new messages */
-	CAMEL_SERVICE_LOCK (store, connect_lock);
 	if (store->current_folder != dest ||
 	    camel_folder_summary_count (dest->summary) == count)
 		camel_folder_refresh_info (dest, ex);
-	CAMEL_SERVICE_UNLOCK (store, connect_lock);
 	
 	if (delete_originals) {
 		for (i = 0; i < uids->len; i++)

Michael Zucchi <notzed ximian com>
"born to die, live to work, it's all downhill from here"
Novell's Evolution and Free Software Developer

Follow-Ups:
- Re: [evolution-patches] fix for bug #61551
  - From: Jeffrey Stedfast

References:
- [evolution-patches] fix for bug #61551
  - From: Jeffrey Stedfast
- Re: [evolution-patches] fix for bug #61551
  - From: Not Zed
- Re: [evolution-patches] fix for bug #61551
  - From: Jeffrey Stedfast

[Date Prev][Date Next] [Thread Prev][Thread Next] [Thread Index] [Date Index] [Author Index]