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

[Not Zed <notzed ximian com>]

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);