D18758: Fix ASan error in test_cmakemanager by catching all signals before objects they are delivered to are deleted.

[Milian Wolff]

mwolff added a comment.


  In D18758#481378 <https://phabricator.kde.org/D18758#481378>, @arrowd wrote:
  
  > R32:bd048e67f056b5be25ed57fb2be947444f68c24e <https://phabricator.kde.org/R32:bd048e67f056b5be25ed57fb2be947444f68c24e>
  >
  > In D18758#481339 <https://phabricator.kde.org/D18758#481339>, @mwolff wrote:
  >
  > > so, I've now committed an alternative fix (or so I hope...) see:
  > >
  > >   commit bd048e67f056b5be25ed57fb2be947444f68c24e
  > >   Author: Milian Wolff <mail at milianw.de>
  > >   Date:   Mon Jun 17 22:26:32 2019 +0200
  > >  
  > >       Guard against crashes when IStatus object gets destroyed at bad times
  > >
  >
  >
  > I confirm this fixes the issue for me. Yay, thanks!
  
  
  Awesome, thanks a lot for checking!
  
  >> having looked at the raw diff quickly, I like what I'm seeing. What boilerplate are you referring to?
  > 
  > At `project.cpp:282`, mostly. This construct looks awful to me:
  > 
  >               });
  >               return retPromise;
  >           }
  >           return QtPromise::resolve();
  >       });
  >    
  >       return retPromise;
  >   }
  >   return QtPromise::resolve();
  >    
  > 
  > And it gets only worser with more indirection. In first version of this patch I made a mistake here, forgot a `return` statement, and this made QtPromise silently not to resolve inner future. Coming from Haskell, I was thinking there should be better way to handle nested futures.
  
  Two things I want to mention here:
  
  First, if you compare this code to any alternative that isn't based on promises, you should hopefully agree that it's still way more readable than any alternative. At least that's my current gut feeling, but I haven't yet worked extensively with QtPromise.
  
  Secondly, some of the boiler plate could potentially be simplified if we add a helper function that takes a KJob, wraps it in a promise and starts the job, but also connects to the error signal to fail the promise? I don't think it will help the "branching" in the promise chain, but at least it should simplify the code a bit to make it clear what everything is doing. Something like the following pseudo code:
  
    startJob(stat project file)
       .fail(show error dialog)
       .then(return startJob(stat developer file))
       .fail(return startJob(stat developer dir))
       .fail(return startJob(create developer dir))
       .then(return startJob(copy project file))
       .fail(show error dialog)
       .then(return startJob(copy developer file))
  
  I'm not sure chaining works like this, so potentially some of this needs to be branched for the error handling, yet overall it should be quite OK and not too much different from the non-async code I believe? Food for thought mostly, we first need to figure out whether we want this at all or not...

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D18758

To: arrowd, #kdevelop, mwolff
Cc: apol, kfunk, brauch, mwolff, kdevelop-devel, hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190618/071c2708/attachment.html>