snu-quiqcl / qiwis

QuIqcl Widget Integration Software
MIT License
5 stars 2 forks source link

Fix the error that the bus thread is not quited #96

Closed BECATRUE closed 1 year ago

BECATRUE commented 1 year ago

So far, when the thread is finished, we only quit it. However, the application is stopped so quickly, there is no enough time for bus thread to finish quit. And it cause the error: Destroyed while thread is still running.

Thus, I just called wait() after quit().

BECATRUE commented 1 year ago

I re-created the PR.

kangz12345 commented 1 year ago

Hmm.. I don't understand why this resolves the issue.

I think Bus.stop() is not called when the program exits, is it?

kangz12345 commented 1 year ago

Could you check if Bus.stop() or Swift.destroyBus() works well?

BECATRUE commented 1 year ago

Could you check if Bus.stop() or Swift.destroyBus() works well?

I just checked that Swift.destroyBus() works well.

BECATRUE commented 1 year ago

Hmm.. I don't understand why this resolves the issue.

I think Bus.stop() is not called when the program exits, is it?

Yes, you're right. Honestly, I'm confused about this, too.

So, I tried to implement a destructor __del__() of swift and call Bus.stop(). In this case, only if we add wait(), it also works well.

I don't know well, but QObject would be destructed automatically when the program exits.

BECATRUE commented 1 year ago

And also, I think QThread works like a daemon thread.

In the documentation,

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 and invoked methods 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 .

I think it means that QThread works like daemon.

kangz12345 commented 1 year ago

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 and invoked methods 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.

This means you should care about where the sender and receiver instances live in. See also: signals and slots across threads.

In my opinion, our problem situation is as follows:

  1. When terminating the program, Bus instances are destroyed without stopping the threads.
  2. When calling Swift.destroyBus(), since bus.stop() immediately returns and does not guarantee that the thread is stopped at all, the following bus.deleteLater() will be executed before the thread actually stops.

I cannot accept the proposed solution to add wait() as a slot of self._consumer.finished... I have no idea why it works. The reason is that self._consumer.finished is a signal that I defined, and it will never be emitted unless the stop() is called so the while loop in run() finishes.

However, I've found a very magical phenomenon. I tested as follows:

  1. Define a new signal QueueConsumer.test().
  2. Connect wait() as a slot to the test signal instead of finished signal.
  3. It also solves the problem! Note that I only defined the signal and never used it, i.e., test signal is never emitted.

😄 I will study this further...

kangz12345 commented 1 year ago

Hmm... maybe the queue consumer is not required at all..?

We might be able to use the "Queued connection" instead.

BECATRUE commented 1 year ago

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 and invoked methods 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.

This means you should care about where the sender and receiver instances live in. See also: signals and slots across threads.

In my opinion, our problem situation is as follows:

  1. When terminating the program, Bus instances are destroyed without stopping the threads.
  2. When calling Swift.destroyBus(), since bus.stop() immediately returns and does not guarantee that the thread is stopped at all, the following bus.deleteLater() will be executed before the thread actually stops.

I cannot accept the proposed solution to add wait() as a slot of self._consumer.finished... I have no idea why it works. The reason is that self._consumer.finished is a signal that I defined, and it will never be emitted unless the stop() is called so the while loop in run() finishes.

However, I've found a very magical phenomenon. I tested as follows:

  1. Define a new signal QueueConsumer.test().
  2. Connect wait() as a slot to the test signal instead of finished signal.
  3. It also solves the problem! Note that I only defined the signal and never used it, i.e., test signal is never emitted.

😄 I will study this further...

The test using QueueConsumer.test() looks so amazing!

Then, why don't you pass this as a temporary solution? I think it will cost a lot of time to not be able to implement testing because of this.

kangz12345 commented 1 year ago

Then, why don't you pass this as a temporary solution?

I think it will cost a lot of time to not be able to implement testing because of this.

It seems reasonable. I will make an issue and approve this PR.