Threading issue that drives me crazy with simplified code
-
Hi guys,
I have been struggling with a code. I have simplified it and you may find the code in the following link:
"Sample code":https://www.dropbox.com/s/y6g5eu6okytsukf/problem.zipThe issue is that, I have main thread and another thread that I create. From the thread, I send a signal named sgRequestCurrentElement to the main thread where I have the slot named onRequestForCurrentElementArrived. In the same slot, I emit a signal back to the thread named sgSendResonseToRequestForCurrentElement and setCurrentElement is called in the thread. In the thread I set the local variable.
The problem is, there is a certain form element in the loaded weblink. It is found everywhere except when I use the local variable in the thread itself. When you run the code, you will see that local variable is set before findFirst is called but there it doesn't find the element anymore.
Please have a look at the code, you will understand what I mean. -
There are couple of things wrong in your code.
The simplest way to see it is if you add << QThread::currentThreadId() to each of your qDebug statements.
First problem - you chose to do threading by subclassing QThread - that's not a bad thing in itself, but you need to know what that means. It means that the run method indeed will be executed in separate thread but the object itself still lives in the main thread.
This has consequences. Because it lives in the main thread any connections you make will be direct connections, not queued. The slots will also fire in the main thread, not the thread run() is sunning in. This causes synchronization problems of course. You're not guarding anything with mutexes etc.
Another problem when you fix that, is that msleep(2000) is not doing what you think it does. You expect that it will process the setCurrentElement() in the meantime, but it won't. It freezes the thread. No slots will be executed at that time. The reason you see it executed is because, as mentioned earlier, it runs in the main thread. The proper way to handle this is to have an event loop running in your worker thread instead of sleep.
I'm not also sure that manipulating QWebElement in another thread is safe, but you'd have to check that.
-
Hi,
If you want to communicate using signals and slots, don't subclass QThread. Create a worker object instead, and move that to a different thread.
From the "QThread documentation":http://qt-project.org/doc/qt-5/qthread.html:
"It is important to remember that a QThread instance lives in the old thread that instantiated it, not in the new thread that calls run(). This means that all of QThread's queued slots will execute in the old thread. Thus, a developer who wishes to invoke slots in the new thread must use the worker-object approach; new slots should not be implemented directly into a subclassed QThread."
-
Awesome!
But still have some questions.As you saw in my code, my purpose is to get the element somehow from main thread to my thread. I don't manipulate it, just do some actions depending on what it contains. So for this, I send a signal from my thread to main thread and the main thread responses back to thread with calling a setter in the thread. However, in my thread where I first emit the request signal, I need to wait a bit to receive the message back from main thread. What is the way to follow? I tried QThread::msleep but it just freezes the thread. QMutex does exactly the same. How should I wait till I receive a signal back?
-
There are couple of ways to do it. Sleeping is not a good way because you don't know how long you should wait. If you wait arbitrary amount of time there will always be that one machine that needs a millisecond more ;)
The important thing to remember is that there needs to be something that can process signals in the worker thread - an event loop. So instead of blocking (msleep) start a "QEventLoop":http://qt-project.org/doc/qt-5/qeventloop.html by calling exec(). It will receive the signal from main thread and call the slot. In it you can do your work and exit event loop (by calling exit()). -
So, right after the place where I emit the first signal from my thread to the main thread, I will call exec(). This will create an event loop and wait till exit() is called. I will call exit() once I receive the response back from the Main thread, which is the setter function in the thread. Please correct me if I misunderstood your comment.
-
That sounds about right.
-
[quote author="Chris Kawa" date="1397482579"]That sounds about right.[/quote]
Hi Chris,
I declared a local QEventLoop in my thread. I called exec right after my signal call in the thread and placed exit in the setter function again in the thread. What happens now is that, I see that the slot in the main thread is called, however, my setter is never called anymore according to the my logs. And if I close down the gui, I got a assert failure in QEventLoop::exec(): "internal error", file kernel\qeventloop.cpp, line 225.
-
Ok, but did you also fix the issue of your thread object? As JKSH and I mentioned your object lives in main thread so its slots will not be executed in the worker thread.
-
[quote author="Chris Kawa" date="1397492107"]Ok, but did you also fix the issue of your thread object? As JKSH and I mentioned your object lives in main thread so its slots will not be executed in the worker thread.[/quote]
Yes I fixed everything mentioned here but apparently I am missing something.
I have updated my simplified code accordingly. Could you please have a look at my code? As you will see, loop.exit(); is never called. I thought that it could be my signal-slot connection but slot is fired ok if I delete the QEventLoop implementation."Updated code":https://www.dropbox.com/s/y6g5eu6okytsukf/problem.zip
-
Yeah, ok, now that you separated worker object from the thread you no longer implement run() method. That means that when you start() the thread a default run() is called. Default run() happens to cal exec(), which creates an event loop.
Your mpStart slot is called already in that loop.So as it is my previous advice about event loop no longer stands. You're already in the loop.
I would just split the mpStart method and execute the second part in your response slot. No need to wait explicitly.
