[Evolution-hackers] New implementation of camel_imap_folder_fetch_data
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);
Re: [Evolution-hackers] New implementation of camel_imap_folder_fetch_data
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.
b
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;
Re: [Evolution-hackers] New implementation of camel_imap_folder_fetch_data
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.
b
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);
Re: [Evolution-hackers] New implementation of camel_imap_folder_fetch_data
So, after the lock fixes and a few other bugfixes, this is a new
implementation.
I left the partial-retrieval thingy in place. Just remove the full
parameter from the function and comment the else part of the if (full)
{ } else { } thingy to use it in a normal Camel.
static void
handle_freeup (CamelImapStore *store, gint nread, CamelException *ex)
{
if (nread = 0)
{
if (errno == EINTR)
camel_exception_set (ex, CAMEL_EXCEPTION_USER_CANCEL,
_(Operation cancelled));
else
camel_exception_setv (ex,
CAMEL_EXCEPTION_SERVICE_UNAVAILABLE,
_(Server unexpectedly
disconnected: %s),
g_strerror (errno));
camel_service_disconnect (CAMEL_SERVICE (store), FALSE, NULL);
}
}
CamelStream *
camel_imap_folder_fetch_data (CamelImapFolder *imap_folder, const char *uid,
const char *section_text, gboolean cache_only,
gboolean full, 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_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);
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 (full)
{
gboolean first = TRUE, err=FALSE;
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] != ' '))
{
err=TRUE;
break;
} 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,
