[PATCH] Fix ThreadWeaver JobCollection deletion race

Robert Knight robertknight at gmail.com
Wed Mar 26 18:30:58 UTC 2008


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.
> 





More information about the KDevelop-devel mailing list