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

Use self-pipe instead of signal-blocking for SIGCHLD/SIGWINCH #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rianhunter
Copy link

pselect() is broken on macOS. This isn't officially documented
anywhere but there are hints of it in this article:

https://daniel.haxx.se/blog/2016/10/11/poll-on-mac-10-12-is-broken/

To safely handle SIGCHLD/SIGWINCH without blocking them,
use the "self-pipe" trick, i.e. write to a pipe when those signals
occur and select() on the read-end of that pipe.

Fixes #19

@rianhunter rianhunter force-pushed the master branch 2 times, most recently from eacb581 to 91137e5 Compare September 4, 2017 03:08
dvtm.c Outdated
int flags = fcntl(fd, F_GETFL, 0);
if (flags < 0) return false;
flags = blocking ? (flags&~O_NONBLOCK) : (flags|O_NONBLOCK);
return (fcntl(fd, F_SETFL, flags) == 0) ? true : false;
Copy link

Choose a reason for hiding this comment

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

My C is rusty but it appears we can omit the ternary operator here.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I copy pasted that code so not surprising.

@hakujin
Copy link

hakujin commented Sep 9, 2017

Thanks for opening this PR! If accepted, it'll be exciting to use dvtm on macOS again.

pselect() is broken on macOS. This isn't officially documented
anywhere but there are hints of it in this article:

https://daniel.haxx.se/blog/2016/10/11/poll-on-mac-10-12-is-broken/

To safely handle SIGCHLD/SIGWINCH without blocking them,
use the "self-pipe" trick, i.e. write to a pipe when those signals
occur and select() on the read-end of that pipe.

Fixes martanne#19
@hakujin
Copy link

hakujin commented Dec 9, 2017

@rianhunter I still observe occasional freezing on macOS Sierra (10.12.6) using HEAD with your patch applied. Does this work for you?

edit: see comment below.

@rianhunter
Copy link
Author

@hakujin Hmm with casual testing I'm not seeing any freezing. I'll check on this more later but I'm worried @martanne doesn't care about this patch.

@hakujin
Copy link

hakujin commented Dec 10, 2017

@rianhunter apologies, I was mistakenly testing a version of dvtm aliased to my system install instead of the patched copy. Confirmed this PR works great.

@martanne anything we can do to get this PR merged?

@jteppinette
Copy link

@rianhunter @martanne I can confirm that this patch works on High Sierra (10.13.1) with 311a8c0 (current HEAD).

kergoth added a commit to kergoth/dvtm that referenced this pull request May 12, 2018
kergoth added a commit to kergoth/dvtm that referenced this pull request May 12, 2018
Reverting in favor of martanne#69.

This reverts commit 97f2e0f.
@rianhunter
Copy link
Author

Ping! This PR is necessary for allowing dvtm to work on macOS, and probably other unix like systems as well. pselect() is a bad linuxism.

@rianhunter
Copy link
Author

@martanne ping again :)

@luke-clifton
Copy link
Contributor

Would love to get this merged. I've been using this patch on macOS and linux without issue for a while.

@rianhunter
Copy link
Author

@martanne ping again 🙏

@luke-clifton luke-clifton mentioned this pull request Nov 6, 2018
@luke-clifton
Copy link
Contributor

Some further motivation for the self-pipe trick.

http://cr.yp.to/docs/selfpipe.html

@rianhunter
Copy link
Author

Is this repo still maintained? It would be better to close this PR than ignore it indefinitely if it's not going to be accepted.

@rpmohn
Copy link
Contributor

rpmohn commented Mar 18, 2019 via email

@travankor
Copy link

I'm considering forking and dealing with the pull requests. It's a tool that I live in every single day

+1
I really like dvtm, too, but the lack of maintenance updates will eventually force me to switch to tmux since that one is well maintained.

demonking added a commit to demonking/dvtm that referenced this pull request Oct 21, 2019
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.

Closing a shell causes window to hang on OS X 10.11
6 participants