Re: [Evolution-hackers] Diary replaying on IMAP accounts
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): ...
Re: [Evolution-hackers] Diary replaying on IMAP accounts
On Mon, 2008-01-21 at 13:17 +0100, Philip Van Hoof wrote: 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) To be honest, I have no idea what the side effects of changing that code to return TRUE for the RESYNCING state are. That's one of the problems with a tri-state that isn't really a tri-state :\ As you mentioned, having it return FALSE is clearly not correct and returning TRUE may introduce some bugs :\ As far as I know, there are some bugs regarding offline usage not resyncing properly when going back online, so you may have found the cause. I'll let the current maintainers make a judgment call on whether to accept this into mainline Camel or not (looks like either way, there's gonna have to be some attention given to this area of code) Sorry that I can't be any more concrete than that :( Jeff ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Diary replaying on IMAP accounts
On Mon, 2008-01-21 at 10:45 -0500, Jeffrey Stedfast wrote: On Mon, 2008-01-21 at 13:17 +0100, Philip Van Hoof wrote: [CUT] (sounds like severe to me) To be honest, I have no idea what the side effects of changing that code to return TRUE for the RESYNCING state are. That's one of the problems with a tri-state that isn't really a tri-state :\ Agree As you mentioned, having it return FALSE is clearly not correct and returning TRUE may introduce some bugs :\ As far as I know, there are some bugs regarding offline usage not resyncing properly when going back online, so you may have found the cause. Perhaps, yes. I'll let the current maintainers make a judgment call on whether to accept this into mainline Camel or not (looks like either way, there's gonna have to be some attention given to this area of code) So Matthew, the ball is in your camp now :-)! -- I'll ping Srinivasa by adding in CC too. -- Sorry that I can't be any more concrete than that :( No problem, I understand. It surprised me quite a bit too as I found this one. I didn't expect the problem to be in that area of the code. The bug report leading to this one was also vague :), our bug tester was telling us that it's not right that he could move a message while the client was offline. He expected the UI to block his action in stead. So we defended that this is actually supported and that the message should be stored remotely as soon as the client goes online. We tested this to be sure of our claim, and we saw that the message was not visible for other E-mail clients ... oeps -- 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 ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Diary replaying on IMAP accounts
Philip, On Mon, 2008-01-21 at 17:01 +0100, Philip Van Hoof wrote: On Mon, 2008-01-21 at 10:45 -0500, Jeffrey Stedfast wrote: On Mon, 2008-01-21 at 13:17 +0100, Philip Van Hoof wrote: [CUT] (sounds like severe to me) To be honest, I have no idea what the side effects of changing that code to return TRUE for the RESYNCING state are. That's one of the problems with a tri-state that isn't really a tri-state :\ Agree As you mentioned, having it return FALSE is clearly not correct and returning TRUE may introduce some bugs :\ As far as I know, there are some bugs regarding offline usage not resyncing properly when going back online, so you may have found the cause. Perhaps, yes. I'll let the current maintainers make a judgment call on whether to accept this into mainline Camel or not (looks like either way, there's gonna have to be some attention given to this area of code) So Matthew, the ball is in your camp now :-)! Philip, I wouldn't be favor of taking things late into trunk, with an uncertainty. If you/matt/fejj have confidence on it, I think I'm fine. Hope you got my point. -Srini ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Diary replaying on IMAP accounts
On Tue, 2008-01-22 at 00:17 +0530, Srinivasa Ragavan wrote: Philip, I wouldn't be favor of taking things late into trunk, with an uncertainty. If you/matt/fejj have confidence on it, I think I'm fine. Hope you got my point. I agree, I would prefer adding this post-2.22. Matthew Barnes ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
[Evolution-hackers] Diary replaying on IMAP accounts
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
___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Diary replaying on IMAP accounts
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.
Jeff
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
