--Boundary-00=_9zK9Dcdg/qjm4rf
Content-Type: text/plain;
  charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

Hi,

while trying to find the cause for http://bugs.kde.org/show_bug.cgi?id=3D87=
163=20
"kaddressbook empties resource on some conditions (data lost)", I found=20
several problems in the KABC file plugin.

=46rom looking at my log files, the problem in #87163 seems to be that KDir=
Watch=20
notifies a change in the addressbook file (ResourceFile::fileChanged()),=20
which causes the addresses to be cleared and asyncLoad() to be called. This=
=20
loading apparently never succeeds during KDE shutdown, so the resource=20
remains empty. A little later, the resource is saved again and the file is=
=20
zeroed.

A fix for this is clear()ing the resource only right before loading the fil=
e,=20
after all async IO has succeeded (clearAndLoad() in the patch). Additionall=
y=20
this prevents successive load() or loadAsync() calls from accumulating=20
address entries (unless there are measures against this somewhere else). Fo=
r=20
extra safety, one could make a copy of mAddrMap before clear()ing and=20
restoring it if format->load() doesn't succeed. Should we do this?

=2D "stale temp file detected"
After downloading with KIO, the KTempFile was not deleted, leading to the=20
above debug message upon the next asyncLoad().

=2D always use KSaveFile for saving

=2D make sure to properly stop and start KDirWatch in all cases

=2D Synchronization issues:
Tickets are used to synchronize the saving of files. This should work with=
=20
synchronous loading, but with async saving the ticket is released as soon a=
s=20
asyncSave returns. (See AddressBook::asyncSave()).
I have added a check to abort saving, if it's still in progress, but this=20
obviously only helps this current ResourceFile instance, so a better soluti=
on=20
is needed. Don't know who's affected by this.

Calling load() before loadAsync() has finished might also lead to troubles,=
 so=20
I added a guard for this.

Attached is a patch for the 3.5 branch. I'll also fix the net plugin later,=
=20
which has similar issues.

Any comments?

Cheers
Carsten

--Boundary-00=_9zK9Dcdg/qjm4rf
Content-Type: text/x-diff;
  charset="us-ascii";
  name="kabc.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="kabc.patch"

Index: plugins/file/resourcefile.cpp
===================================================================
--- plugins/file/resourcefile.cpp	(Revision 510236)
+++ plugins/file/resourcefile.cpp	(Arbeitskopie)
@@ -207,6 +207,10 @@ bool ResourceFile::load()
 {
   kdDebug(5700) << "ResourceFile::load(): '" << mFileName << "'" << endl;
 
+  if ( d->mIsLoading ) {
+    abortAsyncLoading();
+  }
+  
   mAsynchronous = false;
 
   QFile file( mFileName );
@@ -215,11 +219,21 @@ bool ResourceFile::load()
     return false;
   }
 
-  return mFormat->loadAll( addressBook(), this, &file );
+  return clearAndLoad( &file );
+}
+
+bool ResourceFile::clearAndLoad( QFile *file )
+{
+  clear();
+  return mFormat->loadAll( addressBook(), this, file );
 }
 
 bool ResourceFile::asyncLoad()
 {
+  if ( d->mIsLoading ) {
+    abortAsyncLoading();
+  }
+    
   mAsynchronous = true;
 
   if ( mLocalTempFile ) {
@@ -244,26 +258,51 @@ bool ResourceFile::asyncLoad()
   return true;
 }
 
+void ResourceFile::abortAsyncLoading()
+{
+  kdDebug(5700) << "ResourceFile::abortAsyncLoading()" << endl;
+
+  if ( d->mLoadJob ) {
+    d->mLoadJob->kill(); // result not emitted
+    d->mLoadJob = 0;
+  }
+
+  delete mLocalTempFile;
+  mLocalTempFile = 0;
+  d->mIsLoading = false;
+}
+
+void ResourceFile::abortAsyncSaving()
+{
+  kdDebug(5700) << "ResourceFile::abortAsyncSaving()" << endl;
+    
+  if ( d->mSaveJob ) {
+    d->mSaveJob->kill(); // result not emitted
+    d->mSaveJob = 0;
+  }
+
+  d->mIsSaving = false;
+}
+
 bool ResourceFile::save( Ticket * )
 {
   kdDebug(5700) << "ResourceFile::save()" << endl;
 
+  if (d->mIsSaving) {
+    abortAsyncSaving();
+  }
+
   // create backup file
   QString extension = "_" + QString::number( QDate::currentDate().dayOfWeek() );
   (void) KSaveFile::backupFile( mFileName, QString::null /*directory*/,
                                 extension );
 
   mDirWatch.stopScan();
-  KSaveFile saveFile( mFileName );
-  bool ok = false;
-  if ( saveFile.status() == 0 && saveFile.file() )
-  {
-    mFormat->saveAll( addressBook(), this, saveFile.file() );
-    ok = saveFile.close();
-  }
 
+  bool ok = saveToFile( mFileName );
   if ( !ok )
     addressBook()->error( i18n( "Unable to save file '%1'." ).arg( mFileName ) );
+
   mDirWatch.startScan();
 
   return ok;
@@ -271,30 +310,44 @@ bool ResourceFile::save( Ticket * )
 
 bool ResourceFile::asyncSave( Ticket * )
 {
-  QFile file( mTempFile );
+  kdDebug(5700) << "ResourceFile::asyncSave()" << endl;
 
-  if ( !file.open( IO_WriteOnly ) ) {
-    emit savingError( this, i18n( "Unable to open file '%1'." ).arg( mTempFile ) );
-    return false;
+  if (d->mIsSaving) {
+    abortAsyncSaving();
   }
 
-  mDirWatch.stopScan();
-  mFormat->saveAll( addressBook(), this, &file );
-  file.close();
+  bool ok = saveToFile( mTempFile );
+  if ( !ok ) {
+    emit savingError( this, i18n( "Unable to save file '%1'." ).arg( mTempFile ) );
+    return false;
+  }
 
   KURL src, dest;
   src.setPath( mTempFile );
   dest.setPath( mFileName );
 
   KIO::Scheduler::checkSlaveOnHold( true );
-  d->mSaveJob = KIO::file_copy( src, dest, -1, true, false, false );
   d->mIsSaving = true;
+  mDirWatch.stopScan(); // restarted in uploadFinished()
+  d->mSaveJob = KIO::file_copy( src, dest, -1, true, false, false );
   connect( d->mSaveJob, SIGNAL( result( KIO::Job* ) ),
            this, SLOT( uploadFinished( KIO::Job* ) ) );
 
   return true;
 }
 
+bool ResourceFile::saveToFile( const QString& file )
+{
+  KSaveFile saveFile( file );
+  if ( saveFile.status() == 0 && saveFile.file() )
+  {
+    mFormat->saveAll( addressBook(), this, saveFile.file() );
+    return saveFile.close();
+  }
+
+  return false;
+}
+
 void ResourceFile::setFileName( const QString &fileName )
 {
   mDirWatch.stopScan();
@@ -328,10 +381,12 @@ QString ResourceFile::format() const
 
 void ResourceFile::fileChanged()
 {
+    kdDebug(5700) << "ResourceFile::fileChanged(): " << mFileName << endl;
+
   if ( !addressBook() )
     return;
 
-  clear();
+//  clear(); // moved to clearAndLoad()
   if ( mAsynchronous )
     asyncLoad();
   else {
@@ -354,23 +409,32 @@ void ResourceFile::downloadFinished( KIO
 {
   d->mIsLoading = false;
 
-  if ( !mLocalTempFile )
+  if ( !mLocalTempFile ) {
     emit loadingError( this, i18n( "Download failed in some way!" ) );
+    return;
+  }
 
   QFile file( mTempFile );
   if ( !file.open( IO_ReadOnly ) ) {
     emit loadingError( this, i18n( "Unable to open file '%1'." ).arg( mTempFile ) );
+    delete mLocalTempFile;
+    mLocalTempFile = 0;
     return;
   }
 
-  if ( !mFormat->loadAll( addressBook(), this, &file ) )
+  if ( !clearAndLoad( &file ) )
     emit loadingError( this, i18n( "Problems during parsing file '%1'." ).arg( mTempFile ) );
   else
     emit loadingFinished( this );
+
+  delete mLocalTempFile;
+  mLocalTempFile = 0;
 }
 
 void ResourceFile::uploadFinished( KIO::Job *job )
 {
+  kdDebug(5700) << "ResourceFile::uploadFinished()" << endl;
+
   d->mIsSaving = false;
 
   if ( job->error() )
Index: plugins/file/resourcefile.h
===================================================================
--- plugins/file/resourcefile.h	(Revision 510236)
+++ plugins/file/resourcefile.h	(Arbeitskopie)
@@ -28,6 +28,7 @@
 
 #include <kabc/resource.h>
 
+class QFile;
 class QTimer;
 
 class KTempFile;
@@ -148,6 +149,11 @@ class KABC_EXPORT ResourceFile : public 
     void unlock( const QString &fileName );
 
   private:
+    bool saveToFile( const QString& file );
+    void abortAsyncLoading();
+    void abortAsyncSaving();
+    bool clearAndLoad( QFile *file );
+
     QString mFileName;
     QString mFormatName;
 

--Boundary-00=_9zK9Dcdg/qjm4rf--