Re: [Evolution-hackers] New implementation of camel_imap_folder_fetch_data

[Philip Van Hoof <spam pvanhoof be>]

Bweachrgg. It seems camel_imap_command_response performs an extra
CAMEL_SERVICE_REC_UNLOCK (store, connect_lock) (for some reason, I can't
figure out why that it is like that, but ... ).

And it looks that the code calling camel_imap_folder_fetch_data assumes
that this unlock happens (at least at some obscure places).

In other words: crazy code. Anyway, adding that extra unlock seems to
fix it (the current implementation below caused some races here).

Just add an extra CAMEL_SERVICE_REC_UNLOCK (store, connect_lock) before
the returning of the stream and in the errorhandler label.

brrrr

ps. The locking code of the imap provider is really, really broken or
done in an absolutely insane way.


On Tue, 2007-01-09 at 02:18 +0100, Philip Van Hoof wrote:
> Hi there,
> 
> I just finished this one, so there's probably a huge amount of bugs in
> it. I know I should first debug them away before posting on mailing
> lists ;). However, it would be nice if some (other) people would test
> this reimplementation (it's a method in camel-imap-folder.c, by the
> way).
> 
> Just take a look at it, compare it with the memory consumption of
> copying everything into a GData after first copying everything into a
> GPtrArray of a CamelImapResponse structure (which is what the original
> does).
> 
> ps. the if (TRUE) {} thing is because in the version for tinymail,
> there's support for partially retrieving messages. The else part
> implements that support (feel free to take a look at tinymail's
> camel-lite for the implementation, it's not a secret or something).
> 
> 
> CamelStream *
> camel_imap_folder_fetch_data (CamelImapFolder *imap_folder, const char *uid,
> 			      const char *section_text, gboolean cache_only,
> 			      CamelException *ex)
> {
> 	CamelFolder *folder = CAMEL_FOLDER (imap_folder);
> 	CamelImapStore *store = CAMEL_IMAP_STORE (folder->parent_store);
> 	CamelStream *stream;
> 	
> 	/* EXPUNGE responses have to modify the cache, which means
> 	 * they have to grab the cache_lock while holding the
> 	 * connect_lock.
> 
> 	 * Because getting the service lock may cause MUCH unecessary
> 	 * delay when we already have the data locally, we do the
> 	 * locking separately.  This could cause a race
> 	 * getting the same data from the cache, but that is only
> 	 * an inefficiency, and bad luck.
> 	 */
> 
> 	CAMEL_IMAP_FOLDER_REC_LOCK (imap_folder, cache_lock);
> 	stream = camel_imap_message_cache_get (imap_folder->cache, uid, section_text, ex);
> 
> 	/* TNY TODO: if (full) Detect retrieval status (if partial refetch) */
> 
> 	if (!stream && (!strcmp (section_text, "HEADER") || !strcmp (section_text, "0"))) 
> 	{
> 		camel_exception_clear (ex);
> 		stream = camel_imap_message_cache_get (imap_folder->cache, uid, "", ex);
> 	}
> 	CAMEL_IMAP_FOLDER_REC_UNLOCK (imap_folder, cache_lock);
> 	
> 	if (stream || cache_only)
> 		return stream;
> 
> 	camel_exception_clear(ex);
> 
> 	CAMEL_SERVICE_REC_LOCK (store, connect_lock);
> 	CAMEL_IMAP_FOLDER_REC_LOCK (imap_folder, cache_lock);
> 
> 	if (!camel_imap_store_connected(store, ex)) {
> 		camel_exception_set (ex, CAMEL_EXCEPTION_SERVICE_UNAVAILABLE,
> 				     _("This message is not currently available"));
> 		CAMEL_IMAP_FOLDER_REC_UNLOCK (imap_folder, cache_lock);
> 		CAMEL_SERVICE_REC_UNLOCK (store, connect_lock);
> 		return NULL;
> 	}
> 	
> 	camel_exception_clear (ex);
> 
>         stream = camel_imap_message_cache_insert (imap_folder->cache, 
> 			uid, full?section_text:"", "", 0, NULL);
> 	if (stream == NULL)
> 		stream = camel_stream_mem_new ();
> 
> 	if (!stream)
> 		goto errorhander;
> 
> 	if (TRUE)
> 	{
> 	    gboolean first = TRUE;
> 	    gchar line[512];
> 	    guint linenum = 0;
> 	    ssize_t nread; 
> 	    CamelStreamBuffer *server_stream = CAMEL_STREAM_BUFFER (store->istream);
> 	    gchar *tag;
> 	    guint taglen;
> 	    gboolean isnextdone = FALSE;
> 
> 	    if (store->server_level < IMAP_LEVEL_IMAP4REV1 && !*section_text)
> 		    camel_imap_command_start (store, folder, ex,
> 			    "UID FETCH %s RFC822.PEEK",uid);
> 	    else
> 		    camel_imap_command_start (store, folder, ex,
> 			    "UID FETCH %s BODY.PEEK[%s]",uid, section_text);
> 
> 	    tag = g_strdup_printf ("%c%.5u", store->tag_prefix, store->command-1);
> 	    taglen = strlen (tag);
> 	    store->command++;
> 
> 	    while (nread = camel_stream_buffer_gets (server_stream, line, 512) > 0)
> 	    {
> 
> 		    /* It might be the line before the last line */
> 		    if (line[0] == ')' && (line[1] == '\n' || (line[1] == '\r' && line[2] == '\n')))
> 		    {
> 			    isnextdone = TRUE;
> 			    continue;
> 		    }
> 
> 		    /* It's the first line */
> 		    if (linenum == 0 && (line [0] != '*' || line[1] != ' '))
> 		    {
> 			    g_free (tag);
> 			    goto errorhander;
> 		    } else if (linenum == 0) { linenum++; continue; }
> 
> 		    /* It's the last line */
> 		    if (!strncmp (line, tag, taglen))
> 			    break;
> 
> 		    camel_seekable_stream_seek (CAMEL_SEEKABLE_STREAM (stream), 0, CAMEL_STREAM_END);
> 
> 		    if (isnextdone)
> 		    {
> 			    camel_stream_write (stream, ")\n", 2);
> 			    isnextdone = FALSE;
> 		    }
> 
> 		    camel_stream_write (stream, line, strlen (line));
> 
> 		    linenum++;
> 	    }
> 
> 	    g_free (tag);
> 	    
> 	    camel_stream_reset (stream);
> 		
> 	} 
> 
> 	CAMEL_SERVICE_REC_UNLOCK (store, connect_lock);
> 	CAMEL_IMAP_FOLDER_REC_UNLOCK (imap_folder, cache_lock);
> 
> 	return stream;
> 
> errorhander:
> 
> 	CAMEL_SERVICE_REC_UNLOCK (store, connect_lock);
> 	CAMEL_IMAP_FOLDER_REC_UNLOCK (imap_folder, cache_lock);
> 
> 	camel_exception_setv (ex, CAMEL_EXCEPTION_SERVICE_UNAVAILABLE,
> 		    _("Could not find message body in FETCH response."));
> 
> 	if (stream)
> 		camel_object_unref (CAMEL_OBJECT (stream));
> 
> 	return NULL;
> }
> 
> 
> 
-- 
Philip Van Hoof, software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://www.pvanhoof.be/blog