Re: [Evolution-hackers] Diary replaying on IMAP accounts

[Philip Van Hoof <spam pvanhoof be>]

On Sat, 2008-01-19 at 10:48 -0500, Jeffrey Stedfast wrote:
> This is why I started looking at dropping CamelDisco* and replacing all
> instances with CamelOffline* - tri-state is awful, dual-state is ftw.
> 
> anyways, if your fix works, go for it.

Hey Jeffrey,

Do you think that this should be committed in upstream Camel too?

I'll pass the question to Matthew too. I'm not sure whether the function
should indeed always return TRUE in case of RESYNCING state. Perhaps
it's better to adapt the return value of the function and replace all
uses of it? Perhaps TRUE in case of RESYNCING is fine?

There are indeed multiple solutions to solve this.

Note that once the diary starts working, you'll have one or two small
bugs in the diary code too (a variable diary->folders becoming NULL at
some point, yet still being used somewhere).

I wonder whether these diaries ever worked? So suddenly making it work
might introduce new bugs in Evolution too.

Note that the behaviour is that the APPEND command will fail, Camel will
react to that failure by reconnecting and forgetting about the diary.

The result can be called data loss .. since the message that was to be
appended is now only locally available, and not appended remotely.

It's kinda hard to spot this bug, as it requires working with another
E-mail client (if you open the message, it'll just work since it's
cached locally, and it'll also be in the summary since the summary
supports temporary UIDs).

(sounds like severe to me)


> On Sat, 2008-01-19 at 15:13 +0100, Philip Van Hoof wrote:
> > Hi there,
> > 
> > When we connect with an IMAP service if we have moved messages while we
> > where offline, the diary's replay function will be utilised.
> > 
> > While this takes place, the store's disco connection state is
> > CAMEL_DISCO_STORE_RESYNCING.
> > 
> > The camel_disco_store_check_online seems to only care about this state
> > being ONLINE. RESYNCING is not seen as being online. It seems that
> > therefore commands like the APPEND one are failing:
> > 
> > 
> > void
> > camel_disco_diary_replay (CamelDiscoDiary *diary, CamelException *ex)
> > {
> > 	...
> > 	camel_folder_append (...)
> > 	... or ...
> > 	camel_folder_transfer_messages_to (...)
> > 	...
> > }
> > 
> > 
> > imap_append_online (CamelFolder *folder, CamelMimeMessage *message,
> > 		    const CamelMessageInfo *info, char **appended_uid,
> > 		    CamelException *ex)
> > {
> > 	...
> > 	do_append (...)
> > 	...
> > }
> > 
> > static CamelImapResponse *
> > do_append (CamelFolder *folder, CamelMimeMessage *message,
> > 	   const CamelMessageInfo *info, char **uid,
> > 	   CamelException *ex)
> > {
> > 	...
> > 	response = camel_imap_command (store, NULL, ex, "APPEND %F%s%s {%d}",
> > 				       folder->full_name, flagstr ? " " : "",
> > 				       flagstr ? flagstr : "", ba->len);
> > 	g_free (flagstr);
> > 
> > 	if (!response) {
> > 		...
> > 			retry ... (but eventually)
> > 			return NULL;
> > 		...
> > 	}
> > 
> > 	...
> > }
> > 
> > 
> > CamelImapResponse *
> > imap_read_response (CamelImapStore *store, CamelException *ex)
> > {
> > 	...
> > 	response->untagged = g_ptr_array_new ();
> > 	while ((type = camel_imap_command_response (store, &respbuf, ex))
> > 	       == CAMEL_IMAP_RESPONSE_UNTAGGED)
> > 		g_ptr_array_add (response->untagged, respbuf);
> > 
> > 	if (type == CAMEL_IMAP_RESPONSE_ERROR) {
> > 		camel_imap_response_free_without_processing (store, response);
> > ---->		return NULL;   <-----
> > 	}
> > 	...
> > }
> > 
> > 
> > CamelImapResponseType
> > camel_imap_command_response (CamelImapStore *store, char **response,
> > 			     CamelException *ex)
> > {
> > 	CamelImapResponseType type;
> > 	char *respbuf;
> > 	int len = -1;
> > 
> > 	if (camel_imap_store_readline (store, &respbuf, ex) < 0) {
> > 		CAMEL_SERVICE_REC_UNLOCK (store, connect_lock);
> > --->		return CAMEL_IMAP_RESPONSE_ERROR; <--- this happens
> > 	}
> > 
> > 	...
> > }
> > 
> > 
> > /* FIXME: please god, when will the hurting stop? Thus function is so
> >    fucking broken it's not even funny. */
> > ssize_t
> > camel_imap_store_readline (CamelImapStore *store, char **dest, CamelException *ex)
> > {
> >         CamelStreamBuffer *stream;
> >         char linebuf[1024] = {0};
> >         GByteArray *ba;
> >         ssize_t nread;
> > 
> >         g_return_val_if_fail (CAMEL_IS_IMAP_STORE (store), -1);
> >         g_return_val_if_fail (dest, -1);
> > 
> >         *dest = NULL;
> > 
> >         /* Check for connectedness. Failed (or cancelled) operations will
> >          * close the connection. We can't expect a read to have any
> >          * meaning if we reconnect, so always set an exception.
> >          */
> > 
> >         if (!camel_imap_store_connected (store, ex))
> > ->>            return -1;  <---
> > 
> > 	...
> > }
> > 
> > 
> > gboolean
> > camel_imap_store_connected (CamelImapStore *store, CamelException *ex)
> > {
> > 	... (lot's of completely funny looking code, but eventually): ...
> > 	camel_disco_store_check_online
> > 	...
> > }
> > 
> > gboolean
> > camel_disco_store_check_online (CamelDiscoStore *store, CamelException *ex)
> > {
> > 	--> Nothing here seems to accept the RESYNCING state as being online <---
> > 
> >         if (camel_disco_store_status (store) != CAMEL_DISCO_STORE_ONLINE) {
> >                 camel_exception_set (ex, CAMEL_EXCEPTION_SERVICE_UNAVAILABLE,
> >                                      _("You must be working online to "
> >                                        "complete this operation"));
> >                 return FALSE;
> >         }
> > 
> >         return TRUE;
> > }
> > 
> > 
> > 
> > note: I have fixed this in my version by doing this:
> > 
> > 
> > gboolean
> > camel_disco_store_check_online (CamelDiscoStore *store, CamelException *ex)
> > {
> > 
> > 	if (camel_disco_store_status (store) == CAMEL_DISCO_STORE_RESYNCING)
> > 		return TRUE;
> > 
> > 	if (camel_disco_store_status (store) != CAMEL_DISCO_STORE_ONLINE) {
> > 		camel_exception_set (ex, CAMEL_EXCEPTION_SERVICE_UNAVAILABLE,
> > 				     _("You must be working online to "
> > 				       "complete this operation"));
> > 		return FALSE;
> > 	}
> > 
> > 	return TRUE;
> > }
> > 
> > 
> > 
> > -- 
> > Philip Van Hoof, freelance software developer
> > home: me at pvanhoof dot be 
> > gnome: pvanhoof at gnome dot org 
> > http://pvanhoof.be/blog
> > http://codeminded.be
> > 
> 
-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be