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

core: improve signal handling #539

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

PaideiaDilemma
Copy link
Contributor

@PaideiaDilemma PaideiaDilemma commented Oct 30, 2024

This PR tries to handle SIGUSR1 and SIGUSR2 via the main thread using signalfd.

I did that cause when we forceUpdateTimers, we are currently not safe in multiple ways.
We don't acquire the timers lock for m_vTimers, which we can't do cause we are in a async signal context.
We call other async signal unsafe functions.
So I tried moving forceUpdateTimers to the main thread (#536), but that has the problem that we need to wake the main thread somehow and that would require async signal unsafe functions again.

Considering hyprlock is event driven using signalfd made sense.

I hope that this fixes #535. Seems like it does not, but this still makes sense to change i think.

Some testing wanted before merging.
Thanks!

@@ -1156,7 +1226,7 @@ zwlr_screencopy_manager_v1* CHyprlock::getScreencopy() {
}

void CHyprlock::attemptRestoreOnDeath() {
if (m_bTerminate || m_sCurrentDesktop != "Hyprland")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any Idea why we didn't try to restore when m_bTerminate is true before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we want to check if we are actually locked instead?
Cause if we aren't there is no point in trying to restore.

Copy link
Member

Choose a reason for hiding this comment

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

uhhh cuz if it crashes after nlock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but terminate does not mean we already unlocked

Copy link
Member

@vaxerski vaxerski Nov 2, 2024

Choose a reason for hiding this comment

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

oh then ok idk

@PaideiaDilemma PaideiaDilemma marked this pull request as draft October 30, 2024 18:42
@PaideiaDilemma
Copy link
Contributor Author

PaideiaDilemma commented Oct 30, 2024

Needs a bit more work

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.

2 participants