Re: Fix ThreadWeaver JobCollection deletion race

Hi,

I noticed another problem in KDevelop.  When a parse job
completes,
BackgroundParser emits parseJobFinished(Job*) and then
schedules
deletion of the Job.  If the connection from
parseJobFinished() to a
receiver (eg. the ProblemWidget) is queued then the Job may
be deleted
before the slot is called.

The simplest fix is to make sure all connections to
parseJobFinished()
are direct and add a note that this must be the case in the
method
documentation.  This could cause problems though with third
party
plugins if they don't respect this.

Regards,
Robert.

On Wed, 2008-03-26 at 15:06 +0000, Robert Knight wrote:
> Hello,
> 
> The attached patch fixes a couple of problems with
deleting
> JobCollection / JobSequence jobs in threadweaver which
were causing
> sporadic failures in threadweaver's DeleteTest and in
KDevelop after a
> parsing job completed.
> 
> Each JobCollection starts a number of
JobCollectionJobRunner sub-jobs
> which are owned by the JobCollection.  The last
JobCollectionJobRunner
> and collection Job finished after the JobCollection's
done() signal had
> been emitted.  If the JobCollection was after its
done() signal had been
> emitted and before its child jobs had finished, it
would also delete its
> JobCollectionJobRunners and crash. 
> 
> ThreadWeaver's API documentation does not make it clear
when it is safe
> to delete a Job.  The ThreadWeaver tests assume that
when the
> ThreadWeaver instance emits a jobDone(Job*) signal then
it is safe to
> delete that job.  KDevelop currently listens for a
JobSequence's
> done(Job*) signal and then starts a timer to delete the
Job 100ms or so
> in the future.  On my system that was not long enough
every 20 runs or
> so a Job would be deleted while ThreadWeaver was still
using it.
> 
> I suggest making it explicit that a Job can be safely
deleted (with
> respect to ThreadWeaver itself) as soon as it emits its
done(Job*)
> signal.  With the attached patch ThreadWeaver conforms
to these
> semantics.
> 
> Any comments?  Okay to commit?
> 
> Regards,
> Robert.
> 


_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 26.03.08 18:30:58, Robert Knight wrote:
> I noticed another problem in KDevelop.  When a parse
job completes,
> BackgroundParser emits parseJobFinished(Job*) and then
schedules
> deletion of the Job.  If the connection from
parseJobFinished() to a
> receiver (eg. the ProblemWidget) is queued then the Job
may be deleted
> before the slot is called.
> 
> The simplest fix is to make sure all connections to
parseJobFinished()
> are direct and add a note that this must be the case in
the method
> documentation.  This could cause problems though with
third party
> plugins if they don't respect this.

IMHO its a really bad idea to expose the jobs pointer via
public api and
advertise usage of the pointer in slots and at the same time
delete the
job object behind the back of the slot-users.

So maybe we should just add this possibility to parse jobs
as well, that
way if somebody queues a parse job he can also make sure he
deletes the
parse job himself if he expects need for the parse-job data
after the
job finished. Thats what I did for KJob to be able to use
the jobs after
the work has been done (in the vcs support).

Andreas

-- 
Try to value useful qualities in one who loves you.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel