Fix ThreadWeaver JobCollection deletion race

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 Wednesday 26 March 2008 22:38:23 Andreas Pakulat wrote:
> 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

Wouldn't the perfect solution here be using
shared-pointers(KSharedPtr) for 
jobs?

The job would be automatically deleted exactly when it isn't
needed any 
more(This also works with queued connections, as long as the
job is given as 
KSharedPtr<Job>).

Greetings, David 

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

Hi,

> Wouldn't the perfect solution here be using
shared-pointers(KSharedPtr) for 
> jobs?

Yes, I think so.  Job itself is part of threadweaver though
and adding a 
new base class (QSharedData) would be BIC.  This could be
done just for KDevelop::ParseJob though,
on the assumption that any child jobs would have the
ParseJob as their parent and be deleted at the 
same time.

No objections on kde-core-devel or here so I'll commit the
fixes to ThreadWeaver itself.

Regards,
Robert.

On Thu, 2008-03-27 at 15:06 +0100, David Nolden wrote:
> On Wednesday 26 March 2008 22:38:23 Andreas Pakulat
wrote:
> > 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
> 
> Wouldn't the perfect solution here be using
shared-pointers(KSharedPtr) for 
> jobs?
> 
> The job would be automatically deleted exactly when it
isn't needed any 
> more(This also works with queued connections, as long
as the job is given as 
> KSharedPtr<Job>).
> 
> Greetings, David 
> 
> _______________________________________________
> KDevelop-devel mailing list
> KDevelop-develkdevelop.org
> https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel


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

On Thu, Mar 27, 2008 at 10:17 AM, Robert Knight
<robertknightgmail.com> wrote:
> Hi,
>
>
>  > Wouldn't the perfect solution here be using
shared-pointers(KSharedPtr) for
>  > jobs?
>
>  Yes, I think so.  Job itself is part of threadweaver
though and adding a
>  new base class (QSharedData) would be BIC.  This could
be done just for KDevelop::ParseJob though,
>  on the assumption that any child jobs would have the
ParseJob as their parent and be deleted at the
>  same time.
>
>  No objections on kde-core-devel or here so I'll commit
the fixes to ThreadWeaver itself.
>
>  Regards,
>  Robert.
>
>
>
>  On Thu, 2008-03-27 at 15:06 +0100, David Nolden
wrote:
>  > On Wednesday 26 March 2008 22:38:23 Andreas
Pakulat wrote:
>  > > 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
>  >
>  > Wouldn't the perfect solution here be using
shared-pointers(KSharedPtr) for
>  > jobs?
>  >
>  > The job would be automatically deleted exactly
when it isn't needed any
>  > more(This also works with queued connections, as
long as the job is given as
>  > KSharedPtr<Job>).
>  >
>  > Greetings, David
>  >
>  > _______________________________________________
>  > KDevelop-devel mailing list
>  > KDevelop-develkdevelop.org
>  > https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

I don't understand what you mean when you say "you'll
make the changes
to Threadweaver itself". Does this mean you're giving
Threadweaver::Job a new base class of QSharedData? or are
you doing
something different?

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

Hi Matt,

I just been committing the patch attached in the original
email which
ensures that JobCollection jobs do not emit their done()
signal until
the jobs in the collection have emitted their done()
signal.

Using shared pointers for jobs is a separate issue and
assuming binary
compatibility is to be maintained, ThreadWeaver::Job cannot
have a new
base class added, although a proxy class could be created if
necessary.

Regards,
Robert.

On Thu, 2008-03-27 at 14:16 -0500, Matt Rogers wrote:
> On Thu, Mar 27, 2008 at 10:17 AM, Robert Knight
<robertknightgmail.com> wrote:
> > Hi,
> >
> >
> >  > Wouldn't the perfect solution here be using
shared-pointers(KSharedPtr) for
> >  > jobs?
> >
> >  Yes, I think so.  Job itself is part of
threadweaver though and adding a
> >  new base class (QSharedData) would be BIC.  This
could be done just for KDevelop::ParseJob though,
> >  on the assumption that any child jobs would have
the ParseJob as their parent and be deleted at the
> >  same time.
> >
> >  No objections on kde-core-devel or here so I'll
commit the fixes to ThreadWeaver itself.
> >
> >  Regards,
> >  Robert.
> >
> >
> >
> >  On Thu, 2008-03-27 at 15:06 +0100, David Nolden
wrote:
> >  > On Wednesday 26 March 2008 22:38:23 Andreas
Pakulat wrote:
> >  > > 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
> >  >
> >  > Wouldn't the perfect solution here be using
shared-pointers(KSharedPtr) for
> >  > jobs?
> >  >
> >  > The job would be automatically deleted
exactly when it isn't needed any
> >  > more(This also works with queued
connections, as long as the job is given as
> >  > KSharedPtr<Job>).
> >  >
> >  > Greetings, David
> >  >
> >  >
_______________________________________________
> >  > KDevelop-devel mailing list
> >  > KDevelop-develkdevelop.org
> >  > https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel
> 
> I don't understand what you mean when you say
"you'll make the changes
> to Threadweaver itself". Does this mean you're
giving
> Threadweaver::Job a new base class of QSharedData? or
are you doing
> something different?
> 
> _______________________________________________
> KDevelop-devel mailing list
> KDevelop-develkdevelop.org
> https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel


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

> I just been committing the patch attached in the
original email which

That should be "just mean committing".

On Thu, 2008-03-27 at 23:03 +0000, Robert Knight wrote:
> Hi Matt,
> 
> I just been committing the patch attached in the
original email which
> ensures that JobCollection jobs do not emit their
done() signal until
> the jobs in the collection have emitted their done()
signal.
> 
> Using shared pointers for jobs is a separate issue and
assuming binary
> compatibility is to be maintained, ThreadWeaver::Job
cannot have a new
> base class added, although a proxy class could be
created if necessary.
> 
> Regards,
> Robert.
> 
> On Thu, 2008-03-27 at 14:16 -0500, Matt Rogers wrote:
> > On Thu, Mar 27, 2008 at 10:17 AM, Robert Knight
<robertknightgmail.com> wrote:
> > > Hi,
> > >
> > >
> > >  > Wouldn't the perfect solution here be
using shared-pointers(KSharedPtr) for
> > >  > jobs?
> > >
> > >  Yes, I think so.  Job itself is part of
threadweaver though and adding a
> > >  new base class (QSharedData) would be BIC. 
This could be done just for KDevelop::ParseJob though,
> > >  on the assumption that any child jobs would
have the ParseJob as their parent and be deleted at the
> > >  same time.
> > >
> > >  No objections on kde-core-devel or here so
I'll commit the fixes to ThreadWeaver itself.
> > >
> > >  Regards,
> > >  Robert.
> > >
> > >
> > >
> > >  On Thu, 2008-03-27 at 15:06 +0100, David
Nolden wrote:
> > >  > On Wednesday 26 March 2008 22:38:23
Andreas Pakulat wrote:
> > >  > > 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
> > >  >
> > >  > Wouldn't the perfect solution here be
using shared-pointers(KSharedPtr) for
> > >  > jobs?
> > >  >
> > >  > The job would be automatically deleted
exactly when it isn't needed any
> > >  > more(This also works with queued
connections, as long as the job is given as
> > >  > KSharedPtr<Job>).
> > >  >
> > >  > Greetings, David
> > >  >
> > >  >
_______________________________________________
> > >  > KDevelop-devel mailing list
> > >  > KDevelop-develkdevelop.org
> > >  > https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel
> > 
> > I don't understand what you mean when you say
"you'll make the changes
> > to Threadweaver itself". Does this mean
you're giving
> > Threadweaver::Job a new base class of QSharedData?
or are you doing
> > something different?
> > 
> > _______________________________________________
> > KDevelop-devel mailing list
> > KDevelop-develkdevelop.org
> > https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel
> 


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