Calling finished() after error() in KIO slaves causes data corruption and crashes

[Vlad]

Hi,

--- David Faure <faure at kde.org> wrote:
> On Sunday 16 March 2008, Vlad wrote:
> > Another solution would be to transform the documentation guidance
> into
> > a rule and try to enforce it.
> 
> That's how we handled it until now.
> All non-crashing slaves do exactly that: emitting nothing else after
> error().

Makes sense, but I think the documentation for SlaveBase::error()
should be stricter, such as:

"This also finishes the job, so you must not call finished after
calling this."

I'll update this message if that's OK. I'll also add a comment to
SlaveBase::finished().

> > However, that is going to be hard 
> > because finding existing instances where finished() is called
> after
> > error() is not trivial. Frankly, I can see how the instinct of KIO
> > slave writers will be to still call finished() after error(), just
> to
> > be safe and make sure it's finished.
> 
> Yes, we could add some robustness there inside slavebase, but I
> don't
> really like "noop" calls, so I guess it would have to print a
> warning
> about "don't do that", too.

Agreed, ignoring a call without printing a warning would just hide
this bug. So how do we know when to print a warning? One possibility
is to correlate each SlaveBase method with the job that requested it.
This could be done by passing the requester job's unique ID (ie.
memory address) as a parameter to the SlaveBase method. Whenever the
SlaveBase method reports back by emitting a signal such as finished()
or error(), it will pass the requester job's ID as a parameter in that
signal. If we detect that the recipient job doesn't match the
requester job's ID, we block the signal and print a warning.

The drawback to this approach is that all kioslaves will have to be
modified to receive and send job IDs. But I think it's worth the
trouble to prevent crashes (and I'll volunteer to make these changes).
Can you think of a better way?

Thanks,
Vlad


      ____________________________________________________________________________________
Be a better friend, newshound, and 
know-it-all with Yahoo! Mobile.  Try it now.  http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ