Re: [Evolution-hackers] New implementation of camel_imap_folder_fetch_data

[Philip Van Hoof <spam pvanhoof be>]

Ah, I figured it out. The camel_imap_command_start places the lock, and
the functions that are usually used with the results unlock it.

owk. 

"* On success, the store's connect_lock will be locked. It will be freed
 * when you call camel_imap_response_free. (The lock is recursive, so
 * callers can grab and release it themselves if they need to run
 * multiple commands atomically.) "


Easy == different


On Tue, 2007-01-09 at 03:47 +0100, Philip Van Hoof wrote:
> 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