Scheduling jobs and waiting for other activities

[chaley]

Working on it...

[chaley]

@kovid, back the changes out. There appears to be a situation where the called and calling threads are the same, in which case it cannot work. I will get back to you if/when I find a solution.

[kovidgoyal]

reverted.

[chaley]

OK, yet again...

It seems that all the done functions with the exception of device_detected are called from the GUI thread. I am not sure how this comes to be, but it is. FunctionDispatcher would lock up in that case, because one thread is to put the request and another thread consume it. If both are the same thread, then the consumer never gets a chance.

Using Dispatcher in this case works, but given that the threads are the same the call to the done function will happen *after* everything else that goes on, including signal catching and the like. This is wrong.

I have pushed changes where FunctionDispatcher checks if the calling and called thread are the same, and if so directly calls done without signals or queues. I traced it with print statements and it did the right thing when the threads were and were not the same. I also tested it (again) multiple times with my sony and got the behavior I expected.

I think it is stable. I suggest that you merge my branch back in and let meme have at it. If it doesn't fix his problems, then back it out (again) and we will need to do more work to see why he is being called out of turn.

[kovidgoyal]

I'm not happy about the changes to FunctionDispatcher. You're comparing the thread that calls __call__ with the thread that calls __init__. What you should be doing (I think) is comparing the thread that calls __call__ with the gui thread, as q.put(res) is called in the GUI thread. Use the is_gui_thread() function to do that.

[kovidgoyal]

I've changed FD to use is_gui_thread(). Given that the calls to job_done are happening in the GUI thread already, I doubt using FunctionDispatcher will make any difference to meme. Presumably, there is another signal chain earlier in the call sequence.

[chaley]

I did that on purpose. The semantics of FunctionDispatcher are that the signal will be run in the thread that created the instance, thus the thread active in __init__. If the calling thread is the same as the creating thread, then the signal will never be received because the thread will be the same. There is no rule that the instantiating thread is the GUI thread.

[chaley]

Originally Posted by kovidgoyal

I've changed FD to use is_gui_thread().

Not convinced this is correct for the reasons I gave earlier.

Given that the calls to job_done are happening in the GUI thread already, I doubt using FunctionDispatcher will make any difference to meme. Presumably, there is another signal chain earlier in the call sequence.

Also not convinced. Using Dispatcher, the done function was called when a queued signal event was processed. In the meantime, the GUI thread went on and did its thing, running the rest of the functions and starting more jobs. I would imagine that event processing is serial, but there is no reason to believe it is strictly ordered.

[kovidgoyal]

While I agree that the comment of FunctionDispatcher claims that it calls the function in the thread that it was created in, the situation isn't that clear cut.

From the Qt docs at http://doc.qt.nokia.com/latest/threa...across-threads

Queued Connection The slot is invoked when control returns to the event loop of the receiver's thread. The slot is executed in the receiver's thread.

That, in this case, means the thread the FD object was created in as the comment claims. However, if the thread does not have an event loop (which is almost always the case for all non GUI threads in calibre), I'm guessing it is actually called in the GUI thread.

Perhaps a good solution is to raise an exception in FD.__init__ if the current thread is not the GUI thread.

[chaley]

Originally Posted by kovidgoyal

Perhaps a good solution is to raise an exception in FD.__init__ if the current thread is not the GUI thread.

That is a good idea. Clearly the intent is that the FD be instantiated on a thread with an event loop. There is supposed to be only one of those, so checking that invariant seems excellent. It also makes the changes to check for is_gui_thread completely correct, because the only possible signal recipient is that thread.

I assume you will make the change...

EDIT: should Dispatcher make the same check? It clearly is making the same assumption.

[kovidgoyal]

I've committed the change. Semantically we should add it to Dispatcher as well, but since it isn't critical (in that it causes no deadlocks) I'd rather leave well enough alone.

[chaley]

@meme: your turn.

[kovidgoyal]

@meme: Wait.

@chaley: I haven't made the change to use FunctionDispatcher in gui2.device however. Looking at the code, the done function is called from BaseJob.update which in turn is called in the GUI thread by JobsManager._update()

So changing those to FD wont make any difference.

[chaley]

Originally Posted by kovidgoyal

@meme: Wait.

@chaley: I haven't made the change to use FunctionDispatcher in gui2.device however. Looking at the code, the done function is called from BaseJob.update which in turn is called in the GUI thread by JobsManager._update()

So changing those to FD wont make any difference.

Yes it will. Dispatcher uses a (edit: queued) signal no matter what, meaning done is called 'later', while FD (now) will call done synchronously. Or am I misunderstanding your comment?

[kovidgoyal]

It depends on whether JobsManager calls update() before starting the next job or not. I'm trying to figure out whether that is the case.