Issues with setupMatchSession/matchSessionComplete in RunnerManager

[Ryan P. Bitanga]

On Thu, Jul 30, 2009 at 2:04 AM, Aaron J. Seigo<aseigo at kde.org> wrote:
> On Wednesday 29 July 2009, Ryan P. Bitanga wrote:
>> On Thu, Jul 30, 2009 at 12:43 AM, Aaron J. Seigo<aseigo at kde.org> wrote:
>> > On Wednesday 29 July 2009, Ryan P. Bitanga wrote:
>> so I think the best place to do this would be in
>> the destructor of FindMatchesJob. We'd check if the queue is empty,
>> and if it is then we can emit the teardown signal.
>
> there'd be some corner cases there, such as if there are no FindMatchesJobs
> when teardown is requested ... how about in RunnerManagerPrivate::jobDone()?
> see attached patch...

I like the fact that this keeps everything in RunnerManager instead of
having the logic separated between files. Doesn't this suffer from the
same problem though? If there are no jobs to begin with, the slot
won't be called. :( I initially thought the checkTeardown call in
RunnerManager::reset would fix this, but reset is called _before_ the
dialog is hidden so no signal will be emitted in that particular
corner case.

I think an easy way around this would be to enqueue a dummy
FindMatchesJob (with an invalid context to skip the run method?) when
requesting a teardown.

I also think RunnerManager::reset should only deal with having a blank
search context (but necessarily ending the match session) since the
interfaces also call reset when the query changes to an empty string.
In the other times RunnerManager::reset() is called, the dialog is
about to be closed anyway so matchSessionComplete will be called.

> the only downside to this approach in general is that one long-running runner
> (or never returning, even) could indefinitely delay teardown happening for
> other runners. it would be possible to do per-runner bookkeeping if this every
> becomes a real issue.
>
True. I suppose this would be the best solution since there still is
no way to gracefully terminate a thread that's executing a job.

>> > there'd be no point to these signals at all then and runners would just
>> > do all their data gathering in (or just before) match(). that would be
>> > too slow. the point of these signals is to do the setup and teardown as
>> > rarely as possible, but also only when the data will be used.
>>
>> Agreed, but the use case that came to mind while I was writing this
>> e-mail is that of the window runner. It caches the data once a query
>> is launched but the data may become invalid while the dialog is open
>> (e.g. an application is launched/closed while the dialog is open). But
>> this is an issue addressable in the runner implementation.
>
> yes; in this case the setup/teardown lets the runner know that it should start
> watching for those changes as well. if the data can't be trusted between runs
> (meaning that there is no way to detect changes and the data may change) then
> the runner will have to re-load the data in match no matter what we do :)
>
Just for clarity I think we should put that in the apidocs. :)

- Ryan