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

[Jeffrey Stedfast <fejj ximian com>]

approved && committed to CVS

On Mon, 2003-06-02 at 01:06, Not Zed wrote:
> ok so my initial assessment was wrong, and this probably is the nicest
> way to do it at the moment.
> 
> In the longer term, we should probalby have an 'online' signal on the
> session, and let the store automagically update itself based on that. 
> But even that method will need some careful thinking as you can get all
> the same races anyway.
> 
> So attatched is a new patch
>  - it fixes the race and makes the store_online call async cancellable
> as a bonus.
>  - it doesn't remove the online setting in mail-offline-handler.  this
> leads to warnings from the shell about adding existing folders, but is
> non-fatal.  Otherwise offline/online isn't going to work right.
>  - it cleans up the code-path (albeit with uglier logic) to only two
> possibilities, rather than 4 (3 copies of 1 plus the other).
> 
>  Z
> 
> On Sat, 2003-05-31 at 06:46, Jeffrey Stedfast wrote:
> > This only ever happens the first time you run Evolution and if you add
> > an IMAP server as your first account.
> > 
> > The problem is that there is a race condition between the session going
> > online, the stores going online, and the stores getting 'noted' causing
> > a get_folderinfo
> > 
> > it seems that most of the time, the new imap account is in OFFLINE mode
> > in mail_note_store() and so mail_get_folderinfo() never gets called for
> > it.
> > 
> > The attached patch seems to fix it for me, but I'm not sure if it is the
> > best solution or if it introduces other race conditions? (note that I
> > now add a 'struct _store_info' pointer to 'struct _update_data', there
> > may be a race caused by that - not really sure. Actually, hmm... looks
> > like there definetely is.
> > 
> > what I've done is move the mail_set_offline() async function call out of
> > mail-offline-handler.c:storage_go_online() and into mail_note_store()
> > instead, only getting called if the DiscoStore is in offline mode while
> > the CamelSession is in online mode.
> > 
> > This patch still needs work (ugh, introduces another race) but at least
> > it documents what the problem in 43862...
> 
> ______________________________________________________________________
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
> retrieving revision 1.2741
> diff -u -3 -r1.2741 ChangeLog
> --- ChangeLog	2 Jun 2003 01:35:53 -0000	1.2741
> +++ ChangeLog	2 Jun 2003 04:55:56 -0000
> @@ -1,3 +1,19 @@
> +2003-06-02  Not Zed  <NotZed Ximian com>
> +
> +	** This and jeffs patch for #43862.
> +
> +	* mail-folder-cache.c (store_online_cb): If the store is still
> +	around, then flow on to a get folderinfo update, otherwise just clear up.
> +
> +	* mail-ops.c (mail_store_set_offline): return the msgid of this so
> +	it can be cancelled.
> +
> +2003-05-30  Jeffrey Stedfast  <fejj ximian com>
> +
> +	* mail-folder-cache.c (mail_note_store): If the session is
> +	'online' and we are noting a CamelDiscoStore, make sure that it is
> +	changed to online status and call mail_get_folderinfo().
> +
>  2003-05-29  Not Zed  <NotZed Ximian com>
>  
>  	* Makefile.am (BUILT_SOURCES): added server_DATA (*.server) so
> Index: mail-folder-cache.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/mail-folder-cache.c,v
> retrieving revision 1.68
> diff -u -3 -r1.68 mail-folder-cache.c
> --- mail-folder-cache.c	12 Nov 2002 14:59:34 -0000	1.68
> +++ mail-folder-cache.c	2 Jun 2003 04:55:56 -0000
> @@ -42,6 +42,7 @@
>  #include "mail-mt.h"
>  #include "mail-folder-cache.h"
>  #include "mail-ops.h"
> +#include "mail-session.h"
>  
>  /* For notifications of changes */
>  #include "mail-vfolder.h"
> @@ -640,7 +641,7 @@
>  struct _update_data {
>  	struct _update_data *next;
>  	struct _update_data *prev;
> -
> +	
>  	int id;			/* id for cancellation */
>  
>  	void (*done)(CamelStore *store, CamelFolderInfo *info, void *data);
> @@ -799,6 +800,25 @@
>  	return TRUE;
>  }
>  
> +static void
> +store_online_cb (CamelStore *store, void *data)
> +{
> +	struct _update_data *ud = data;
> +
> +	LOCK(info_lock);
> +
> +	if (g_hash_table_lookup(stores, store) != NULL) {
> +		/* re-use the cancel id.  we're already in the store update list too */
> +		ud->id = mail_get_folderinfo(store, update_folders, ud);
> +	} else {
> +		/* the store vanished, that means we were probably cancelled, or at any rate,
> +		   need to clean ourselves up */
> +		g_free(ud);
> +	}
> +
> +	UNLOCK(info_lock);
> +}
> +
>  void
>  mail_note_store(CamelStore *store, EvolutionStorage *storage, GNOME_Evolution_Storage corba_storage,
>  		void (*done)(CamelStore *store, CamelFolderInfo *info, void *data), void *data)
> @@ -849,16 +869,27 @@
>  		camel_object_hook_event(store, "folder_unsubscribed", store_folder_unsubscribed, NULL);
>  	}
>  
> -
> -	if (!CAMEL_IS_DISCO_STORE (store) ||
> -	    camel_disco_store_status (CAMEL_DISCO_STORE (store)) == CAMEL_DISCO_STORE_ONLINE ||
> -	    camel_disco_store_can_work_offline (CAMEL_DISCO_STORE (store))) {
> +	/* We might get a race when setting up a store, such that it is still left in offline mode,
> +	   after we've gone online.  This catches and fixes it up when the shell opens us */
> +	if (CAMEL_IS_DISCO_STORE(store)
> +	    && camel_session_is_online(session)
> +	    && camel_disco_store_status (CAMEL_DISCO_STORE (store)) == CAMEL_DISCO_STORE_OFFLINE) {
>  		ud = g_malloc(sizeof(*ud));
>  		ud->done = done;
>  		ud->data = data;
> -		ud->id = mail_get_folderinfo(store, update_folders, ud);
> -		
> -		e_dlist_addtail(&si->folderinfo_updates, (EDListNode *)ud);
> +		/* Note: we use the 'id' here, even though its not the right id, its still ok */
> +		ud->id = mail_store_set_offline (store, FALSE, store_online_cb, ud);
> +
> +		e_dlist_addtail (&si->folderinfo_updates, (EDListNode *) ud);
> +	} else if (!CAMEL_IS_DISCO_STORE(store)
> +		   || camel_disco_store_status (CAMEL_DISCO_STORE (store)) == CAMEL_DISCO_STORE_ONLINE
> +		   || camel_disco_store_can_work_offline (CAMEL_DISCO_STORE (store))) {
> +		ud = g_malloc (sizeof (*ud));
> +		ud->done = done;
> +		ud->data = data;
> +		ud->id = mail_get_folderinfo (store, update_folders, ud);
> +
> +		e_dlist_addtail (&si->folderinfo_updates, (EDListNode *) ud);
>  	}
>  
>  	UNLOCK(info_lock);
> Index: mail-ops.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/mail-ops.c,v
> retrieving revision 1.391
> diff -u -3 -r1.391 mail-ops.c
> --- mail-ops.c	19 May 2003 12:44:31 -0000	1.391
> +++ mail-ops.c	2 Jun 2003 04:55:57 -0000
> @@ -2269,12 +2269,13 @@
>  	set_offline_free,
>  };
>  
> -void
> +int
>  mail_store_set_offline (CamelStore *store, gboolean offline,
>  			void (*done)(CamelStore *, void *data),
>  			void *data)
>  {
>  	struct _set_offline_msg *m;
> +	int id;
>  
>  	/* Cancel any pending connect first so the set_offline_op
>  	 * thread won't get queued behind a hung connect op.
> @@ -2289,7 +2290,10 @@
>  	m->data = data;
>  	m->done = done;
>  
> +	id = m->msg.seq;
>  	e_thread_put(mail_thread_queued, (EMsg *)m);
> +
> +	return id;
>  }
>  
>  /* ** Execute Shell Command ***************************************************** */
> Index: mail-ops.h
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/mail-ops.h,v
> retrieving revision 1.59
> diff -u -3 -r1.59 mail-ops.h
> --- mail-ops.h	19 May 2003 12:44:32 -0000	1.59
> +++ mail-ops.h	2 Jun 2003 04:55:57 -0000
> @@ -156,9 +156,9 @@
>  void mail_prep_offline(const char *uri, CamelOperation *cancel,
>  		       void (*done)(const char *, void *data),
>  		       void *data);
> -void mail_store_set_offline(CamelStore *store, gboolean offline,
> -			    void (*done)(CamelStore *, void *data),
> -			    void *data);
> +int mail_store_set_offline(CamelStore *store, gboolean offline,
> +			   void (*done)(CamelStore *, void *data),
> +			   void *data);
>  
>  /* filter driver execute shell command async callback */
>  void mail_execute_shell_command (CamelFilterDriver *driver, int argc, char **argv, void *data);
-- 
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
fejj ximian com  - www.ximian.com