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

Fix: signal handler musn't depend on the event loop #15325

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Jan 7, 2025

Only a limited set of POSIX functions are signal safe, and the system functions that the event loop implementations can rely on isn't in the list (e.g. epoll, kevent, malloc, ...).

Now, the writer side of the pipe is blocking, so we should never reach a nonblocking case that would trigger an event loop wait, but going to the event loop may still be doing far too much or dangerous things: an event loop might not be available (e.g. bare thread) and it might be lazily allocated (signal unsafe).

Only a limited set of POSIX functions are signal safe, and the system
functions that the event loop implementations can rely on isn't in the
list (e.g. epoll, kevent, malloc, ...).
@ysbaddaden ysbaddaden added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:system platform:unix labels Jan 7, 2025
@ysbaddaden ysbaddaden self-assigned this Jan 7, 2025
@ysbaddaden
Copy link
Contributor Author

Aside: it's becoming more and more evident that we should move on to signalfd and EVFILT_SIGNAL, and overall to move all the signal logic into the Crystal::EventLoop API, so they can each use what would be best for them.

@straight-shoota straight-shoota added this to the 1.16.0 milestone Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:unix topic:stdlib:system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants