Skip to content

Handle invalid poll descriptors in device loop - #88

Closed
JeffreyBenjaminBrown wants to merge 1 commit into
monome:mainfrom
JeffreyBenjaminBrown:fix-poll-disconnect-spin
Closed

JeffreyBenjaminBrown wants to merge 1 commit into
monome:mainfrom
JeffreyBenjaminBrown:fix-poll-disconnect-spin

Conversation

@JeffreyBenjaminBrown

Copy link
Copy Markdown

Fixes a possible CPU spin when poll reports POLLNVAL after a device or IPC fd becomes invalid. This may address #81. (Together with a parallel PR I am submitting now to libmonome, it reduced CPU usage from around 45% to negligible on my system, which can be inspected here: https://github.com/JeffreyBenjaminBrown/nixos-experiments/tree/xfce/config.)

@tehn
tehn requested a review from artfwo May 15, 2026 15:56
@artfwo

artfwo commented May 15, 2026 •

Copy link
Copy Markdown
Member

Thanks for the PR! It seems to address multiple concerns though and, since I cannot reproduce the problem locally (on a couple of distros with both FTDI and CDC grids), I wonder which specific descriptor flag check (or lack of) on which descriptor is causing the problem in the first place?

@artfwo

artfwo commented Aug 1, 2026

Copy link
Copy Markdown
Member

@JeffreyBenjaminBrown while revisiting this today, I've observed 0x19 state on fd.revents when the grid is plugged out (POLLIN=1 POLLERR=1 POLLHUP=1 POLLNVAL=0)

If you're getting POLLNVAL on grid disconnect with your setup, I'd be happy to merge a version of this change, but I cannot see how the part of it checking state->ipc_in_fd or fds[2] is helping here. What exactly is the case when the supervisor fd becomes invalid?

@JeffreyBenjaminBrown

Copy link
Copy Markdown
Author

Hi Artem. I'm sorry this response took so long. I vibe-coded those two PRs, didn't know what I was doing, and was in over my head. You seem to have been right that the logic was flawed. I'm closing the libmonome patch I submitted as irrelevant, and (below) offering what I think is a good fix. The problem has vanished on my system -- I can unplug a monome without causing serialosc activity to ramp up and stay at 100% of a CPU until closing it. (The old fix seemed to solve it but then it came back.)

Even the new fix is something I don't understand, but I did my best to get the AI (Codex) to provide a succinct description and justification. (I tried to edit this PR in the browser, but Github's interface is complex and this is just a four-line change, so instead I'm simply pasting the diff below.)

Thanks for the time you spent on this issue, and for your open source work more generally.

The rest of this message is by the AI.

The revised patch

diff --git a/wscript b/wscript
--- a/wscript
+++ b/wscript
@@
 	code = """
 		#include <stdlib.h>
+		#include <fcntl.h>
 		#include <poll.h>

diff --git a/src/serialosc-device/event_loop/select.c b/src/serialosc-device/event_loop/select.c
--- a/src/serialosc-device/event_loop/select.c
+++ b/src/serialosc-device/event_loop/select.c
@@
-		if (FD_ISSET(monome_fd, &rfds))
-			monome_event_handle_next(state->monome);
+		if (FD_ISSET(monome_fd, &rfds)
+				&& monome_event_handle_next(state->monome) < 0)
+			break;

Reasoning

serialosc's configure test calls open() without including fcntl.h, the
header that declares it. Modern compilers reject the implicit declaration
under -Werror. The failed test is interpreted as a broken poll(), so
serialosc builds event_loop/select.c even though poll() works.

On Linux, unplugging the grid made the tty readable at EOF, but did not mark it
in select()'s exceptional-descriptor set. The select loop therefore called
monome_event_handle_next(). libmonome detected POLLERR | POLLHUP and
returned an error, but serialosc ignored that return value and immediately
repeated the loop, consuming one CPU core.

The first change makes the configure test valid, restoring the intended poll
event loop. That loop already handles POLLERR | POLLHUP. The second change
also repairs the select fallback by terminating when libmonome reports a read
error.

Live verification

Before

With a monome 256 connected at /dev/ttyUSB0, unplugging it left
serialosc-device alive and spinning at high CPU. A trace repeatedly showed:

read(3</dev/ttyUSB0>(deleted), "", 1) = 0
poll([{fd=3, events=POLLIN}], 1, 25)
  = 1 ([{fd=3, revents=POLLIN|POLLERR|POLLHUP}])

The build log also showed the erroneous fallback:

Checking for working poll()              : no (will use select())

After

With the patch applied, configuration reported:

Checking for working poll()              : yes

The build compiled event_loop/poll.c. The patched package was installed and
tested with the same monome. Before unplug, its serialosc-device process was
idle at 0% CPU. On unplug, /dev/ttyUSB0 disappeared and the process exited
promptly. The supervisor logged:

serialosc [m0000102]: disconnected, exiting

@artfwo

artfwo commented Aug 27, 2026

Copy link
Copy Markdown
Member

ok, so there are 2 problems here.

one is that poll() test code in wscript has an implicit open() that modern compilers treat as an error. this has already been fixed in cmake files for a while, but waf needs that too, otherwise it falls back to select() on linux.

the second problem is that select event loop get stuck in a loop of monome_event_handle_next calls on linux. that loop works fine on macos though, probably because the call to select() itself fails when the device is unplugged and we don't need to handle the monome_event_handle_next return value at all.

i'm going to manually add missing includes for poll() tests, that alone should fix the problem on linux and look into the 2nd part, as time allows (as poll is the supported path for linux).

please feel free to reopen the issue if the problem persists, thanks for getting back to us with the results!

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