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

Switch from polling on FIFOs to signal #12

Merged
merged 12 commits into from
Nov 22, 2019
Merged

Switch from polling on FIFOs to signal #12

merged 12 commits into from
Nov 22, 2019

Conversation

geohot
Copy link
Collaborator

@geohot geohot commented Nov 19, 2019

First off, remove randomness unless there's a real justification for it. In general, unless you absolutely need it, randomness shouldn't be used.

Secondly, with uids being pids, I think the signalling solution is obvious. The polling process can sleep until the publishing process decides it's time to wake it up. I don't know the nuance here, but usleep can be exited by EINTR, though I think the signal handler might have to be configured to do that.

You could get more fine grained with the poll, where the subscriber says if it's blocked on a given socket to avoid spurious wakeups (right now it wakes on all subscribed sockets), but this is a perf optimization for later and maybe not needed at all.

Right now this segfaults with python demo.py, and I don't know why. In general, this software needs a much better test suite, it underlies all of system stability and has to deal with many different orders of events.

@geohot geohot requested a review from pd0wm November 19, 2019 21:18
@pd0wm
Copy link
Contributor

pd0wm commented Nov 19, 2019

Sounds like nanosleep is the way to go here. It is designed to be interrupted, has nice exit codes and even stores the remaining time when interrupted in a new struct.

http://man7.org/linux/man-pages/man2/nanosleep.2.html

@pd0wm pd0wm merged commit 347a866 into master Nov 22, 2019
@pd0wm pd0wm deleted the signal branch February 24, 2020 23:55
esalaverria1 pushed a commit to esalaverria1/cereal that referenced this pull request Jan 12, 2023
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