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)
{
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 (stor
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
[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
