Skip to content

Use threading.Lock instead of QMutex to workaround QMutexLocker bug in PyQt6 #151

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danielrh
Copy link

This solves #128

@dnadlinger
Copy link
Contributor

Looks good to me. There doesn't seem to be any reason to use QMutex[Locker] in the first place – when recently fixing some of the threading issues, I just left it in because it was already in use in the previous version. I'd guess (famous last words) that just using threading.Lock is faster too.

@dnadlinger
Copy link
Contributor

dnadlinger commented Aug 18, 2025

Unsurprisingly – now that we have @danielrh's root cause analysis showing that this is a bug in QMutexLocker independent of qasync –, this indeed seems to have fixed all qasync-related crashes in the deployments here.

It would be good to cut a release with this as a matter of urgency, as this greatly improves the stability on Windows. (I'm not sure whether it was the recent switch to Windows 11 in the deployment here or upgrading the PyQt/Qt versions, but the current state is almost unusably unstable for our applications.)

@dnadlinger
Copy link
Contributor

@hosaka: In case reviewing the state of master regarding a new release takes more bandwidth than you have to spare right now, could we potentially just backport this to 0.27.1 and release it as a follow-up 0.27.2? (Given that I had a close look into the threading model in #87, I'm confident in saying that this is at least as correct as the previous implementation that hits the PyQt bug.)

@hosaka
Copy link
Collaborator

hosaka commented Aug 20, 2025

Hi, thanks for the PR. Can we get a rebase on top of master to resolve the conflicts? And yeah it's about time we make a new release.

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.

3 participants