dwm freezes on SIGTERM when using the seamless reload patch

[UtkarshVerma]

trafficstars

When I enable the seamless reload patch, doing the pkill -TERM dwm freezes my PC and I have to manually quit X through another VT console.

[bakkeby]

Does this also happen when you use the keybinding to restart dwm?

[bakkeby]

Ref. https://dwm.suckless.org/patches/restartsig/ the TERM one is to exit dwm cleanly. I take it that this issue happens when you want to quit the window manager.

[UtkarshVerma]

Yes, this is when I try to quit dwm through pkill -TERM dwm.

[bakkeby]

Tried reproducing this without luck.

Typically this happens when there is something else other than dwm holding the X session alive, but that shouldn't be related to the seamless reload patch. Try checking what other applications are running and try to kill those one by one (rather than X itself) until the X session terminates naturally. If you find the culprit then you could add that to the kill script.

[N-R-K]

Think I might know what the problem is. You cannot just call any function inside a sighandler, see man 7 signal-safety.

In this case however, the quit function doesn't call anything and only sets running to 0 and restard to (possibly) 1. Which means two things:

0. running and restard need/should to be declared as volatile and changed from int to sig_atomic_t.
1. It still may not solve the problem.

To elaborate on the 2nd point, let's say you receive the while inside a XNextEvent() call, the sighandler will set running to 0 and then you'll go back to sleeping/blocking inside the XNextEvent call. And the restart would only occur after the next X event has been received and handled, not instantly.

[UtkarshVerma]

To elaborate on the 2nd point, let's say you receive the while inside a XNextEvent() call, the sighandler will set running to 0 and then you'll go back to sleeping/blocking inside the XNextEvent call. And the restart would only occur after the next X event has been received and handled, not instantly.

Yes, this behaviour does happen, and it would be really great if we could fix this. Having to wait a couple of seconds after pkill -TERM dwm every time does not feel good. I use rofi to shutdown/restart my laptop hence I am stuck with using signals instead of direct bindings.

[bakkeby]

Yes, this behaviour does happen

I haven't seen any proof of this @UtkarshVerma, or any investigation on your own part.

Does the bar show when it is frozen for example? Is dwm running when you connect to another VT console to kill the X session? Does it use any CPU?

[UtkarshVerma]

@bakkeby The last reply is unrelated to the freezing issue. It was simply another quirk I noted. Freezing issue I haven't found time to investigate yet.

[bakkeby]

You can try to see if the above change makes any difference for you

[UtkarshVerma]

Thanks for the commit. dwm doesn't freeze anymore.

[UtkarshVerma]

This is a demo of the restartsig patch having delayed effects.

https://user-images.githubusercontent.com/31820255/178232812-0eef7ac5-2573-44c4-92e9-d2ee0a1818e5.mp4

[bakkeby]

It may be something else interfering. What is the command being run from that menu? pkill -HUP dwm?

I notice that your bar stopped updating - why would that be?

[N-R-K]

and it would be really great if we could fix this.

It can be fixed by polling on the X fd before calling XNextEvent. Here's an example on how to do it from sxcs and here's another example from herbe.

[N-R-K]

https://github.com/bakkeby/dwm-flexipatch/commit/d4ab4400ac4ac0744341113f7329614403c8fcb4

Hmm, I was only looking at the restartsig patch and didn't notice flexipatch has other stuff going on in quit as well.

Looking at the diff, kill and waitpid are both listed as async-signal-safe so those should be fine. I suspect persistmonitorstate() was the problem then, which was probably calling Xlib functions (none of which are signal-safe afaik).

Btw, I'd still change running and restard's type from int to volatile sig_atomic_t.

[N-R-K]

Untested, but should work I think. Try it out @UtkarshVerma

From 003115538a7dcbe8bc3c634c3ca99482fd8549cf Mon Sep 17 00:00:00 2001
From: NRK <[email protected]>
Date: Mon, 11 Jul 2022 21:16:20 +0600
Subject: [PATCH] call XNextEvent only when there's data to be read

---
 dwm.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/dwm.c b/dwm.c
index 0bdd36c..6437024 100644
--- a/dwm.c
+++ b/dwm.c
@@ -261,7 +261,7 @@ static void (*handler[LASTEvent]) (XEvent *) = {
 	[UnmapNotify] = unmapnotify
 };
 static Atom wmatom[WMLast], netatom[NetLast];
-static int running = 1;
+static volatile sig_atomic_t running = 1;
 static Cur *cursor[CurLast];
 static Clr **scheme;
 static Display *dpy;
@@ -1380,11 +1380,24 @@ void
 run(void)
 {
 	XEvent ev;
-	/* main event loop */
 	XSync(dpy, False);
-	while (running && !XNextEvent(dpy, &ev))
+	/* main event loop */
+	while (running) {
+		struct pollfd pfd = {
+			.fd = ConnectionNumber(dpy),
+			.events = POLLIN,
+		};
+		int pending = XPending(dpy) > 0 || poll(&pfd, 1, -1) > 0;
+
+		if (!running)
+			break;
+		if (!pending)
+			continue;
+
+		XNextEvent(dpy, &ev);
 		if (handler[ev.type])
 			handler[ev.type](&ev); /* call handler */
+	}
 }
 
 void
-- 
2.35.1

[UtkarshVerma]

Sorry for getting back at this so late. I pulled changes from master yesterday and this seems to have been fixed.

[UtkarshVerma]

I didn't test is properly, I think. The issue is still there.

[N-R-K]

I didn't test is properly, I think. The issue is still there.

Which one? Freezing or the restard being delayed?

[UtkarshVerma]

delayed restart. I'll test out your patch today

[N-R-K]

delayed restart. I'll test out your patch today

I think today ended 3 days ago.

But in any case, this probably should be a separate issue since the original issue here was solved.

[UtkarshVerma]

Yeah, sorry about that. Things have been a bit hectic recently. I will create a new issue for this.

[UtkarshVerma]

@N-R-K thanks, the patch works fine. Hope it gets merged. The new issue is #295