413165 - Autosave IMAP mail message drafts should not save when sending message

Status: NEW

(MailNews Core :: Networking: IMAP, defect)

Description

[Kyle Guinn]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20070601 SeaMonkey/1.1.2 Mnenhy/0.7.4.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20070601 SeaMonkey/1.1.2 Mnenhy/0.7.4.0

I am trying to send an email with a large attachment over a dialup connection (i.e. it's slow) on an IMAP account.  During the sending process, mailnews tries to autosave the message, which uses more (unavailable) bandwidth and sometimes causes sending the message to fail.  More often, mailnews gives an error that it cannot copy the message to the Sent folder.

Reproducible: Sometimes

Steps to Reproduce:
1. Enable autosaving message drafts
2. Send a large message (e.g. a large attachment) on an IMAP account over a slow connection (e.g. dialup), where the expected time to send the message is greater than the autosave interval.
Actual Results:  
Mailnews attempts to save the message draft during mailing, which fails due to lack of bandwidth.  I am unable to tell if the message was actually sent or sent correctly.

Expected Results:  
Mailnews should not autosave when mailing.

Comment 1

[Igor Velkov]

You can just set drafts and snt folders to local folders.

Comment 2

[Serge Gautherie (:sgautherie)]

Kyle,

Can you reproduce with SeaMonkey v1.1.9 ?


(In reply to comment #1)
> You can just set drafts and snt folders to local folders.

(That should be a workaround.)

Comment 3

[Kyle Guinn]

(In reply to comment #2)
> Kyle,
> 
> Can you reproduce with SeaMonkey v1.1.9 ?
> 

I am no longer on dial-up so I cannot effectively test this bug.

Comment 4

[Wayne Mery (:wsmwk)]

pretty sure this is a dupe
https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=draft&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&product=MailNews+Core&product=Thunderbird&product=Toolkit&resolution=---&bug_severity=major&bug_severity=normal&bug_severity=minor&emailreporter1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=

Comment 5

[hansen]

I can only find bug 380275. Not sure if I should dupe, since they're in different components, but they seem to be the issue.

Comment 6

[Wayne Mery (:wsmwk)]

Nikolay or bienvenu, is this something you could take a crack at? 
(bug 380275 has quite a few dups)

Comment 7

[David :Bienvenu]

Hmm, weird. We have a global var gSendOrSaveOperationInProgress and if that's true, we don't do autosave.

Comment 8

[Wayne Mery (:wsmwk)]

wada is there really a difference between this bug and bug 380275?

Comment 9

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #8)
> wada is there really a difference between this bug and bug 380275?

bug 380275 is for "mail send while auto-save is in progress", per bug summary. i.e. That bug is complaint of "mail send while auto-save is in progress".
IIRC, bug reports for "auto-save shouldn't be schedulued once mail send is requested" exist.

Comment 10

[Wayne Mery (:wsmwk)]

(In reply to David :Bienvenu from comment #7)
> Hmm, weird. We have a global var gSendOrSaveOperationInProgress and if
> that's true, we don't do autosave.

Parkhideh, can you test this? No rush

Comment 11

[Parkhideh]

(In reply to Wayne Mery (:wsmwk) from comment #10)
> (In reply to David :Bienvenu from comment #7)
> > Hmm, weird. We have a global var gSendOrSaveOperationInProgress and if
> > that's true, we don't do autosave.
> 
> Parkhideh, can you test this? No rush

Thunderbird 17.0.6

1) Problem one is that I do not know what "autosave" parameter is and where to find it.  I assume that it is not " Options \ Composition \ General \ Auto Save every " parameter because that should be a local activity.

2) Second problem is that I have never used the NewsGroup features.  If this is related to it I need help.

3) What I did test is to choke the system to see if shortage of bandwidth causes any errors.  It did not.  What was tested:
   a) Attached a large file to IMAP folder and sent.
   b) While sending, moved a large file from an IMAP folder to a different IMAP account and folder.  It requires an inherent download and up load.
   c) Opened three tabs on Firefox 7.0.1; each took over several minutes where it normally takes a few seconds.
   d) Since all worked, although many minutes later; it dawned on me that the test was not valid because all manual commands in Thunderbird could have been queued.  :)  The original report complains about AutoSave.

   So, if each manual command creates a thread, then the problem is almost not there; if manual commands are queued, then someone should tell me how to setup the autosave.

Comment 12

[WADA:World Anti-bad-Duping Agency]

(In reply to David :Bienvenu from comment #7)
> Hmm, weird. We have a global var gSendOrSaveOperationInProgress and if
> that's true, we don't do autosave.

Even if so, following is pretty normal in multi-task environment.
     Task-1                           Task-2
     Mail sending                     Auto save
         gSendOrSaveOperationInProgress = false
                                      new auto-save interval starts
     Start mail sending
                                      If g...InProgress==false ...
     g...InProgress=true
                                      Because of false, start auto-save
     mail sending continues
     This is a "auto-save is invoked after start of mail sending".
although I think major cases is "mail send is requested while auto-save is in progress" or "mail send is requested just after start of new auto-save interval".
To serialize "mail sending" and "auto-save", I think mutex lock like mechanism is needed in multi-task environment especialy when multi-cpu environment.

Comment 13

[Parkhideh]

In the spirit of wanting to see this bug removed, and yet keeping the tongue-in-cheek humour; I will make a comment and ask a question or two:

My Comment: Comments 7 and then 12 came as a surprise.  A multitasking system (Thunderbird) that is single channeled.  You see we used this technique when the I/O was slow and hence they were the bottle neck.  After all, what choice did we have but to stand in line?
But look at my situation today;  I travel with Thunderbird on the stick (I mean Flash, like U3 or portable apps).  Running Thunderbird off the Flash, into a 3+ GHz PC with a 1G bit LAN connection to a mail server that probably has four processors.  Where do you think the bottle neck is?  The answer is USB 2.0.  So we all now wait for the fetch and store to the flash while other processes could get through an empty LAN!

Now my question: There is this feature that checks my mail every minute to tell me if friends are going for lunch.  (See bug 714576)
1- Does this feature also get blocked by this global parameter? i.e. I shouldn't start my large "send" until I hear from friends?  :)
2- If yes, does that have the mutex lock?

Comment 14

[WADA:World Anti-bad-Duping Agency]

(In reply to Parkhideh from comment #13)
> My Comment: Comments 7 and then 12 came as a surprise.

Why you surprized by them?
That is;
 David(developer) says "mail send and auto-save doesn't occur at same
 time, because a global var gSendOrSaveOperationInProgress is used".
 So, I simply asked him "Really?",

> A multitasking system (Thunderbird) that is single channeled.

What do you mean?

As David says, "send mail" perhaps sets gSendOrSaveOperationInProgress=true, and auto-save has logic like "if( gSendOrSaveOperationInProgress==true{ don't do auto-save; }.
However, such simple checking/setting value is never perfect for serialization in multi-tasking(example in comment #12 is one of most basic issue in multi-tasking, and anyone perhaps knows about it if he learned about "multi-tasking" pretty a littel).
My question was: Really? Is "Read a var, then set a value" by two tasks actually serialized any time, any place in Tb? "Read a var which is 0 by Task-1", "Read a var which is 0 by Task-2", "Check value==0 by Task-2, and set value-2 by Task-2", "Check value==0 by Task-1, and set value-1 by Task-1", never occurs in Tb?
Or "mail send" and "auto-save" is executed by a single process/task/thread of Tb serially?

"send mail" in Tb actually has logic like "MailIsModifiedFlag=false" upon start of "send mail" to avoid auto-save after start of mail send. And, according to comment in source, needles to say, "auto-save" side has logic like (A) "If mail is not modified, don't invoke auto-save" or logic like (B) "If mail is not modified, don't do auto-save action even if auto-save is invoked". Sorry but I don't  know (A) or (B) or (A)&(B).

My question is also:
- If gSendOrSaveOperationInProgress==true by "send mail" can perfectly
  prevent "auto-save after start of mail send" in any case,
  why such clearing of "mail is modified" state is needed before start
  of mail sending?
- If gSendOrSaveOperationInProgress==true by auto-save can perfectly
  prevent "mail send while auto-save  sis in progress, or just after
  invocation of next auto-save interval"?

Currently, serialization between "mail send" and "manual draft save" is virtually achieved by "UI lock" or "Seriluzation at UI level".
  - If "Send" is requested, "Save As Draft" menu/button can not be
    clicked at UI.
  - If "Save As Draft" is requested, "Send Mail" menu/button can not be
    clicked at UI.

My question is also a question to developer:
  Is gSendOrSaveOperationInProgress==true checked at any place in
  auto-save code as you expect or you assume?

My comment #12 is also question to bug opener and all "me too" posters in this bug:
- Is your problem actually "auto-save is invoked after mail send is
  requested"? (bug summary says so.)
- Your problem is "mail send is requested while auto-save is running",
  which is already reported to other bugs,
  isn't it?

Comment 15

[Parkhideh]

(In reply to WADA from comment #14)
> 
> Why you surprized by them?
> That is;
>  David(developer) says "mail send and auto-save doesn't occur at same
>  time, because a global var gSendOrSaveOperationInProgress is used".
>  So, I simply asked him "Really?",
> 
First, thank you for your explanation and quick response.
Next I will split my probing into two comments for clarity.

This is a repeated question, because I did not find a response in comment 14:
**- In the option to checks mail every minute:
Q1- Does or should this feature also get blocked by global parameter gSendOrSaveOperationInProgress? 
Q2- If yes, does it have a lock mechanism as you have suggested in comment 12?

Comment 16

[Parkhideh]

(In reply to WADA from comment #14)
> (In reply to Parkhideh from comment #13)
> > My Comment: Comments 7 and then 12 came as a surprise.
> 
> Why you surprized by them?
> That is;
>  David(developer) says "mail send and auto-save doesn't occur at same
>  time, because a global var gSendOrSaveOperationInProgress is used".
>  So, I simply asked him "Really?",
> 
> > A multitasking system (Thunderbird) that is single channeled.
> 
> What do you mean?
> 
This is what I meant in comment 13, and why I was surprised.

In a multitasking environment why does one task have to wait for a channel to the mail server?  As far as I know, and correct me if this is not the case, the mail server will allow many channels to be opened to the same account.**  So if my statement and test is correct, why does a task have to check gSendOrSaveOperationInProgress parameter.  When a task needs a channel it should open it, use it and close it at will, without affecting other tasks.
The advantage of this approach is one less global parameter; and in a case of conflict, the mail server knows best how to commit the action.
That was my surprise, why Thunderbird is carrying the burden of possible conflicting user requests?


** I have tested this with having Thunderbird, a POPMAIL Delphi and a POP3 Visual Basic program, all accessing one mail account, concurrently from a single IP address.

Comment 17

[WADA:World Anti-bad-Duping Agency]

(In reply to Parkhideh from comment #15)
> This is a repeated question, because I did not find a response in comment 14:
> **- In the option to checks mail every minute:
> Q1- Does or should this feature also get blocked by global parameter
> gSendOrSaveOperationInProgress? 

I couldn't understand why you refer to "New mail check" which is irrelevant to gSendOrSaveOperationInProgress(as named, it's obviously for "Mail Send" and "Save to Drafts") in this bug for "Mail Send" and "Auto Save"...

Answer to Q1 = No.
Contension between "IMAP server access for new mail fetch" and "IMAP server access for sent mail copy to IMAP Sent or draft save to IMAP Drafts" is absolutely independent problem from this bug.
And, because default of max cached connection is 5, "new mail check at IMAP INBOX", "Upload to IMAP Sent", "Upload to IMAP Drafts", are executed at different connection. So, problem of this bug is not contension between "Upload to IMAP Sent" and "Upload to IMAP Drafts".
Problem is contension between "Delete of last draft mail after mail send" and "Delete of old version of draft after auto-save".
This contensin occurs even when local Drafts folder. So, it's never IMAP only issue. And, it's not contension of a cached connection for IMAP Drafts folder.

Comment 18

[Parkhideh]

(In reply to WADA from comment #17)
> (In reply to Parkhideh from comment #15)
> > This is a repeated question, because I did not find a response in comment 14:
> > **- In the option to checks mail every minute:
> > Q1- Does or should this feature also get blocked by global parameter
> > gSendOrSaveOperationInProgress? 
> 
> I couldn't understand why you refer to "New mail check" which is irrelevant
> to gSendOrSaveOperationInProgress(as named, it's obviously for "Mail Send"
> and "Save to Drafts") in this bug for "Mail Send" and "Auto Save"...
> 
> Answer to Q1 = No.
> Contension between "IMAP server access for new mail fetch" and "IMAP server
> access for sent mail copy to IMAP Sent or draft save to IMAP Drafts" is
> absolutely independent problem from this bug.
Thank you again for the explanation.  Good to know that different channels are used by each operation.  AND in case my inquiry appears as reporting a bug; that is NOT the case.

Now back to comment 12.  The serialization will not solve the problem.  This in particular happens in multi CPU environment.  Look at the following steps:
1- Task Send gets a copy and is ready to send.
2- Task AutoSave gets a copy and is ready to save.
3- Task Send gets the channel.
4- Task AutoSave is in lock hold waiting for the channel.
5- Task Send completes.  Sends and deletes from draft.
6- Task Send releases the channel.
7- Task AutoSave gets the channel and saves a copy in draft.

If above is the dilemma, then the solution is not in channel lock, although that is important.  There is a missing step 5a after 5:
5a- If Send is successful then KILL task AutoSave and Restart it.  Now AutoSave cannot get a copy of the old draft.  I assume here that there is no way to have two copies of a draft because after the completion of previous AutoSave the Delete should have happened.

Comment 19

[WADA:World Anti-bad-Duping Agency]

(In reply to Parkhideh from comment #18)
> The serialization will not solve the problem.

Why?
"Serialization" means "send mail"(==SMTP send, copy to Sent, delete draft from Drafts) and "auto-save"(==save new draft to Drafts, delete old draft from Drafts)" won't occur at same time.
i.e. Following works well.
  "send mail" : upon start of "send mail", if "auto-save" is running, 
                wait for completion of "auto-save".
  "auto-save" : upon next auto-save interval, or upon start of "auto-
                save" process, if "send mail" is already started,
                don't invoke "auto-save" process.
One of purposes of gSendOrSaveOperationInProgress is obviously above, and I'm suspecting "current logic around gSendOrSaveOperationInProgress" doesn't work as expected or as designed if "auto-save" is relevant.

> 5a- If Send is successful then KILL task AutoSave and Restart it.

FYI.
IMAP process has queuing mechanism. So, following may happen.
  auto-save saves new draft to Drafts.
  auto-save queue IMAP request of "select Drafts" if not selected,
  "uid store xx +Flag(\Deleted)" for old draft.
     <= Killing auto-save task at here is pretty problematic.
        Many code is perhaps required for checking status of many
        elements in order to kill already running "auto-save" task.
  auto-save waits for completion of "uid store xx +Flag(\Deleted)"
  command by IMAP task.

> This in particular happens in multi CPU environment.

FYI.
Problem in multi-tasking can occur even on single CPU(or single "Thread"), because recent multi-tasking is preemptive muti-tasking even in OS for PC.
  - Any task can be halted at any step.
  - Order of re-scheduling of halted tasks is usually uncontrollable,
    even if many OSes uses FIFO like scheduling in re-scheduling.
Multi-CPU simply increases probability of "such problem actually occurs in actual environment" sufficiently.
Needless to say, "multi-CPU only problem"(==never occurs if single CPU) surely exists.

Comment 20

[Parkhideh]

This is not a reply to comment 18 because I have to go through it carefully.

1)
The reason for my objection to locking a channel is the following scenario:
a- SEND task locks the channel and takes 20 minutes to send.
b- User meanwhile has edited few more drafts.  I assume this is possible.
c- Network disconnect occurs.  None of the data is saved.

So what is needed is a flag, set by Task SEND that keeps AutoSave Task away from ONE mail that is being sent;  but not to interfere with its saving of other drafts.

There is another problem, this one because I am afraid of ghosts. :)
I do not like or approve of data channel locks that do not have an expiration mechanism.  If one of the SEND or AutoSave tasks dies or is caught in a loop, the other is also history.  Not a happy situation.

Now the problem is, there is no way to predict the period needed for an upload.  Putting a heart beat on tasks is not in the scope of Thunderbird.  So a short verifiable lock (duration of execution) or a messaging technique like
d- From SEND TASK to AutoSave TASK:  Reserving e-mail #n.
e- From AutoSave TASK to SEND TASK:  #n is set aside  OR  #n is in use.
may do with minimal code and with OS protection.
I am sure you know better ways to flag a number for another task without creating a race condition.

2)
I agree with you that number of CPU will only increase the probability of hitting a race condition, when one exists.

3)
I don't see why I should be concerned with how the tasks are invoked, yet.  But last I looked there was a priority assigned to tasks (even if it was low/high in some scheduler queues) that had some guarantees on who would NOT start before another.  My preference is also assuming that they run randomly.

4)
Finally, yes Killing a Task has its problems; but only if it is in an unknown state.  In the algorithm presented in comment 12, which halts the task when channel is not available, killing AutoSave task is easy because it is always in the same state.  
It should be clear that if item 1) above is implemented then AutoSave task is always running while it keeps away from the ONE mail that is "reserved" by the SEND task.

Comment 21

[Parkhideh]

(In reply to WADA from comment #19)
> (In reply to Parkhideh from comment #18)
> > The serialization will not solve the problem.
> 
> Why?
> "Serialization" means "send mail"(==SMTP send, copy to Sent, delete draft
> from Drafts) and "auto-save"(==save new draft to Drafts, delete old draft
> from Drafts)" won't occur at same time.

Now I have absorbed what was said; with one point that was not mentioned.  That is when SEND task queues its SMTP send and requests DELETE draft; does the control require a handshake similar to "waits for completion of "uid store xx +Flag(\Deleted)" that was mentioned for AutoSave TASK?

In the following I assume that the answer is either YES or subsequent IMAP accesses will not see the deleted file (even if it is only flagged for deletion).  If this is not the case please explain, don't read the rest and send me back to the drawing board.  :)

My proposal:
A) Goal: 
-To have each task its own data channel; And expecting them to run concurrently;
-no serialized locking mechanism (by using hold back parameters under control of the programmer)
-no task killing; 
-expecting random start and suspense of tasks by the scheduler; 
-trading race problem with a collision that statistically will have low probability due to "read only" and "write only" usage of inter-locking parameters.  
   (And I do hope this is not inventing a perpetual motion machine :)

B) Replacing gSendOrSaveOperationInProgress parameter with two parameters
   B1- gSendEmailID  which is write only for SEND TASK and read only for AutoSave.
   B2- gAutoSaveEmailID  which is read only for SEND TASK and write only for AutoSave TASK.

C) Algorithm:

      SEND TASK
Entry Point ST:
 if gAutoSaveEmailID == <my Send email ID>
    gSendEmailID  = 0
    <message on status line, waiting for AutoSave to complete>
    Sleep few scheduler cycles and go to Entry Point ST  (see note 1)
 else
    gSendEmailID  = <my Send email ID>
 end
       // test for collision
 if gAutoSaveEmailID == <my Send email ID>
    gSendEmailID  = 0
    Sleep a few scheduler ticks and go to Entry Point ST
 end
    // Start sending the e-mail
    ..............
    // When done
    gSendEmailID  = 0
    // we are done


                                            AutoSave TASK
                        Entry Point AST:
                        if gSendEmailID  == <my Save email ID>
                           if <no other Auto Save candidates>
                              gAutoSaveEmailID = 0
                              <Reschedule for the next auto save cycle>
                           else
                              <my Save email ID> = <pick the next candidate>
                              <Start at Entry Point AST>
                           end
                        else
                              gAutoSaveEmailID = <my Save email ID>
                        end
                            // test for collision
                        if gSendEmailID  == <my Save email ID>
                           gAutoSaveEmailID = 0
                           // a few scheduler ticks wait does not hurt
                           go to Entry Point ST
                        end
                           // Start Auto Save the e-mail
                            ..............
                           // When done
                           gAutoSaveEmailID = 0
                           // we are done


Note 1)
    It would be user friendly if after an elapsed time or number of cycles, the message became like a prompt allowing the user to cancel SEND and get it out of this loop.  Example would be having to wait many minutes for the AutoSave that started before SEND.  Maybe the user likes to send a different mail while AutoSave is completing the current email upload.