[Nepomuk] Review Request: ResourceWatcher: Do no create a new ResourceManager on quit

[Vishesh Handa]

> On Dec. 5, 2012, 2:16 a.m., Simeon Bird wrote:
> > libnepomukcore/resource/resource.cpp, line 54
> > <http://git.reviewboard.kde.org/r/107573/diff/1/?file=97142#file97142line54>
> >
> >     Might it be better to simply return some empty resource, instead of asserting?
> >     
> >     In 292996 , we just figured out that a Resource can continue being accessed after QApplication has been destroyed, because QRunnable cannot be cancelled. If that thread, instead of accessing the Resource, had constructed a new Resource, with this code we would crash on exit in a release build. 
> >     
> >     Since we would likely only hit the race in a release build, we would get another difficult-to-debug crash on exit.
> 
> Vishesh Handa wrote:
>     Maybe I should remove the assert? That empty resource doesn't really cause any harm. What do you think?
> 
> Simeon Bird wrote:
>     If it were me, I would remove the assert entirely since, as you say, the empty resource doesn't really cause any harm, and if it is left in we could easily get bugs reported from some user accidentally compiling in debug mode and crashing kontact (or whatever). 
>     
>     But it's up to you - I can see that the assert might be useful if it occurred somewhere other than application quit.

Removed the assert, and added a kError() message instead :)


- Vishesh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107573/#review23001
-----------------------------------------------------------


On Dec. 6, 2012, 8:40 a.m., Vishesh Handa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107573/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2012, 8:40 a.m.)
> 
> 
> Review request for Nepomuk, Sebastian Trueg and Simeon Bird.
> 
> 
> Description
> -------
> 
>     ResourceWatcher: Do no create a new ResourceManager on quit
>     
>     The ResourceManager contains a ResourceWatcher. This ResourceWatcher is
>     initialized only when a Resource class requires it. On destruction it
>     calls ResourceWatcher::stop, which tries to disconnect it from
>     ResourceManager::nepomukSystemStarted signal.
>     
>     The problem arises when the application is exiting, and the
>     ResourceManager (being a child of QCoreApplication) is being destroyed.
>     It in turn destroys the ResourceWatcher, which calls
>     ResourceWatcher::stop, which calls ResourceManager::instance() which
>     results in the creation of a new ResourceManager whose parent is
>     QCoreApplication::instance, which is 0, cause the application is
>     shutting down.
>     
>     This whole extra ResourceManager being allocated is not required and can
>     be avoided by checking if QCoreApplication != 0, in
>     ResourceManager::instance.
>     
>     Also, since ResourceManager::instance() can now return 0. I've added an
>     assert check in the Resource class.
> 
> 
> Diffs
> -----
> 
>   libnepomukcore/datamanagement/resourcewatcher.cpp a09b646 
>   libnepomukcore/resource/resource.cpp 4601eba 
>   libnepomukcore/resource/resourcemanager.cpp 457c042 
> 
> Diff: http://git.reviewboard.kde.org/r/107573/diff/
> 
> 
> Testing
> -------
> 
> Added breakpoints in gdb, and wrote a simple application. The extra ResourceManager is no longer being created.
> 
> 
> Thanks,
> 
> Vishesh Handa
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20121209/7d27ab5c/attachment.html>