KDAB / cxx-qt

Safe interop between Rust and Qt
https://kdab.github.io/cxx-qt/book/
1.04k stars 70 forks source link

Add is_alive method to CxxQtThread #984

Open akiselev opened 3 months ago

akiselev commented 3 months ago

When combining async Rust channels and QT objects, it's really easy to end up in a situation where a message is received on the channel after the QObject has already been destroyed, causing CxxQtThread::queue to throw an exception. This PR introduces an is_alive() function to check the handle if it's been invalidated.

LeonMatthesKDAB commented 3 months ago

Hi @akiselev , thank you for taking the time to create this PR!

overall I think the is_alive method should be useful, so it's good to have it. However, depending on what you want to use it for, this may not do what you intend!

If you want to check is_alive before calling queue, this will not work!

e.g. this:

if thread.is_alive() {
    thread.queue(...).unwrap()
}

May panic and crash your program. The reason for this is that the CxxQtThread does not have ownership of the QObject, it is only a handle to the thread the QObject lives in. That other thread may delete the QObject at any time, including between the calls to is_alive() and queue.

So ideally you should really prefer just using.

thread.queue(...).ok();

if you want to avoid crashing your application in that case.

If however, you want to use is_alive() to e.g. exit a loop:

while qobject_thread.is_alive() {
    // do some rust work
    // maybe message the qobject somewhere
    qobject_thread.queue(...).ok();
    // do some more rust work
}

Then it's very much a good and convenient API.

So really you can only know for certain whether a QObject has already been destroyed, not whether it's still alive, as it may be destroyed at any point. Therefore I'd suggest renaming this to is_destructed/is_destroyed/is_dead or something like this and prominently documenting this caveat.

akiselev commented 2 weeks ago

I renamed the method to is_destroyed and added a better doc comment explaining the race condition

LeonMatthesKDAB commented 2 weeks ago

Hi @akiselev , Thank you for the update to the PR. I'm happy to approve the new API, but suspect CI will fail because the tests haven't been updated to the new generated output. If your system supports it, can you run the ./update_expected.sh shell script in the cxx-qt-gen directory? That should automatically update the generation tests.

In case you have issues with the script, I can run it as well, just let me know.

akiselev commented 2 weeks ago

Updated

ahayzen-kdab commented 2 weeks ago

Looks there there is another case of a test needing updating :-)

Note you can run cargo test in the crates/cxx-qt-gen to see the ones that are failing.