search

tenox7 / ttyplot

a realtime plotting utility for terminal/console with data input from stdin
Apache License 2.0
957 stars 42 forks source link

Call `pselect` twice to not starve signal handling (alternative to #130) #132

Closed hartwork closed 7 months ago

hartwork commented 7 months ago

Alternative to #130.

With both aggressive stdin input and signals coming in, pselect sometimes delays signal delivery for multiple seconds, to the point where even holding Ctrl+C down for seconds will not be successful in terminating the process. I have witnessed this symptom for a duration of 20+ seconds in practice.

To reproduce:

# ./ttyplot < /dev/zero
[hold Ctrl+C or press repeatedly]
[see it take multiple seconds for the process to terminate (if at all)]

Related: https://stackoverflow.com/questions/62315082/pselect-on-linux-does-not-deliver-signals-if-events-are-pending

edgar-bonet commented 7 months ago

This has a few issues:

hartwork commented 7 months ago

@edgar-bonet I understand that this is one of the more controversial among the pull requests we had so far, and we don't have to rush this.

Regarding your points:

Personally I would rather rebase #115 on top of this after #132 has been merged; the conflict resolution should be trivial, if it's not I would volunteer for the resolution.

  • It has a race condition for signals delivered between the two pselect calls.

Yes, a race condition that will cause up to ~200ms delay in handling of Ctrl+C compared to 20+ seconds on current development.

  • It makes the program repaint way more often than needed, even in the most common case of reasonable stdin pressure.

Yes, but so far I considered rendering at 5 fps acceptable, we can discus that or revert back to 500ms and be conscious about it.

To summarize, at the moment all these combined seem minor to me when compared to non-functioning Ctrl+C.

hartwork commented 7 months ago

@edgar-bonet I have dropped the increase in painting rate now, it only now only adds the second call to pselect. That will help keeping Ctrl+C working even with high pressure and the race condition will cause a few milliseconds in delay at worst and not do any other damage. I believe this is ready to go, thanks for your feedback and consideration.

hartwork commented 7 months ago

@edgar-bonet I have extracted a function pselect_without_signal_starvation now to restore the feel that there is one single event waiting location in the code. I hope you like it.

hartwork commented 7 months ago

This still has a race condition. But the window of vulnerability is small, so this will do for now. Will probably implement a race-free alternative another day.

@edgar-bonet perfect, thank you! :pray:

hartwork commented 7 months ago

@edgar-bonet rebasing onto latest development for a green CI…