Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cxx-qt-gen: emit signals immediately #281

Merged
merged 3 commits into from
Oct 6, 2022

Conversation

ahayzen-kdab
Copy link
Collaborator

Now that we use a recursive mutex the chance of a deadlock
is much lower and also defined behaviour. So remove the queued
emit signal calls as now we can emit immediately.

@LeonMatthesKDAB
Copy link
Collaborator

This is a tricky one, I'll first have to read up a bit on the exact defnitions of safe/unsafe in Rust.
https://doc.rust-lang.org/nomicon/meet-safe-and-unsafe.html

We shouldn't be able to do something unsafe from safe code 🤔
Which is problematic, because a Signal emission may do anything.

@ahayzen-kdab
Copy link
Collaborator Author

This is a tricky one, I'll first have to read up a bit on the exact defnitions of safe/unsafe in Rust. https://doc.rust-lang.org/nomicon/meet-safe-and-unsafe.html

We shouldn't be able to do something unsafe from safe code thinking Which is problematic, because a Signal emission may do anything.

What would be unsafe ? Eg how is this any different to calling a mutable function ? eg

fn invokable(&mut self) {
  self.field = 1;
  myMutableFunction();  // <- this is the same as our signal? if is a ``mutable method so has the same borrowing rules
  self.field = 2;
}

@LeonMatthesKDAB
Copy link
Collaborator

The difference is just that most slots that are going to be connected to the signal will be implemented in C++ or QML (most likely).
Meaning the code that will be executed by these functions is unsafe.
However, if we continue with our current assumption, that all C++ code is in a giant "unsafe" block, and is therefore responsible for its own problems, this is actually okay to do.

From the Rust book:
"The design of the safe/unsafe split means that there is an asymmetric trust relationship between Safe and Unsafe Rust. Safe Rust inherently has to trust that any Unsafe Rust it touches has been written correctly. On the other hand, Unsafe Rust cannot trust Safe Rust without care."

We just have to trust that these functions have been written correctly.

Interestingly, this brings up a problem that we might have in future, when we work on setting up signal and slot connections from Rust.
There, we pass safe Rust code into something that will later be called from "unsafe" code, so we should do our best to ensure there is no way in which a "safe" rust slot can produce undefined behavior.

Anyway, I'll proceed with the review.

Now that we use a recursive mutex the chance of a deadlock
is much lower and also defined behaviour. So remove the queued
connection calls as now we can emit immediately.
Now that we use a recursive mutex the chance of a deadlock
is much lower and also defined behaviour. So remove the queued
emit signal calls as now we can emit immediately.
Instead use QTRY_COMPARE to wait for the value to change, as on
some machines this may take more than one event loop.
Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LeonMatthesKDAB LeonMatthesKDAB merged commit a92f7b4 into KDAB:main Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change signals to be immediate now that we have a recursive mutex
2 participants