[Kde-pim] Review Request 113340: fix reading of mbox file when slowly filled by external program

[Martin Koller]

> On Feb. 6, 2014, 7:30 p.m., Kevin Krammer wrote:
> > I assume the unittest still run?
> > Do you think you could create a unit test that fails without this and works with this change?

Not entirely sure if running the tests is correct how I did it.
I just started ./kmbox/tests/testmbox and it runs some tests, which end with:
Totals: 12 passed, 0 failed, 0 skipped
(I get 2 warnings: KMBox::MBox::setLockType: Could not find the mutt_dotlock executable
which I think do not matter here)

I have now added a unittest also.


> On Feb. 6, 2014, 7:30 p.m., Kevin Krammer wrote:
> > kmbox/mbox_p.cpp, line 68
> > <https://git.reviewboard.kde.org/r/113340/diff/1/?file=203431#file203431line68>
> >
> >     does it hurt to leave it here?
> >     The other line could then have a commend like
> >     // update file size in case it has changed while we were waiting for the lock.

It does not hurt but it does not make sense either.
The flow is:
MBox::load()
 d->initLoad()  which previously set mInitialMboxFileSize = mMboxFile.size();
 lock()
...

Now I moved the mInitialMboxFileSize = mMboxFile.size(); AFTER the lock().
initLoad() ist a member of a private class and is not called from anywhere else.

One could even completely get rid of MBoxPrivate::initLoad() and move the code
after the lock(), except mMboxFile.setFileName() which is needed in lock()
Would that be better ?


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/113340/#review49150
-----------------------------------------------------------


On Feb. 7, 2014, 9:03 a.m., Martin Koller wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/113340/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2014, 9:03 a.m.)
> 
> 
> Review request for KDEPIM-Libraries and Bertjan Broeksema.
> 
> 
> Bugs: 324173
>     http://bugs.kde.org/show_bug.cgi?id=324173
> 
> 
> Repository: kdepimlibs
> 
> 
> Description
> -------
> 
> The mbox resource can watch for file changes.
> When an external program fills the mbox file and this takes some time (e.g. 10 seconds, during which mails get added and added),
> the file watcher triggers indirectly the method MBox::load(), which does as a first step d->initLoad( fileName ); (mainly to set the filename
> so the following lock() call knows what the lockfile shall be).
> But the bug is that the d->initLoad( fileName ) call also stores the current mbox filesize - which is still changing until the lock can be finally made.
> (I'm talking about procmail lockfile). When the external program is finally done, the mbox filesize is larger than what the mbox resource
> has stored in d->mInitialMboxFileSize
> 
> This patch stores the filesize AFTER the lock could be made.
> 
> 
> Diffs
> -----
> 
>   kmbox/tests/mboxtest.h 992ab36 
>   kmbox/tests/mboxtest.cpp ed2c079 
>   kmbox/mbox.cpp 3658bf6 
>   kmbox/mbox_p.cpp b4e323b 
> 
> Diff: https://git.reviewboard.kde.org/r/113340/diff/
> 
> 
> Testing
> -------
> 
> The following shell script shows the problem (you also need a file "testmail" which contains a mail to be added).
> E.g. make sure the mbox file is empty, the run the script. When the loop is done, kmail will tell you there are 5 new mails, but
> only the first one shows subject, sender, etc.
> 
> #!/bin/bash
> 
> num=$RANDOM
> i=0
> 
> LOCKFILE=~/airsync.mbox.lock
> lockfile -l20 -r5 $LOCKFILE
> if [ $? -ne 0 ]
> then
>   echo "locking error"
>   exit
> fi
> 
> while (( $i < 5 ))
> do
>   let num++
>   let i++
>   echo "sending mail" $num
>   sed -e "s/<NUMBER>/$num/" testmail >> ~/airsync.mbox
>   ls -l  ~/airsync.mbox*
> 
>   sleep 3
> done
> 
> rm -f $LOCKFILE
> 
> 
> Thanks,
> 
> Martin Koller
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/