fix(libc): correct transient-errno predicate in js_waker_signal#1480
Conversation
The retry loop in js_waker_signal used `errno != EAGAIN || errno != EINTR`, which is always true (any single errno value is unequal to at least one of the two), so the loop bailed on the first short-write error and silently dropped the wakeup. Replace with `&&` so the loop only exits on a fatal errno. No test: js_waker_signal's transient-error path is reached only when the pipe is full or the syscall is interrupted by a signal, neither of which is reliably reproducible in a portable unit test.
| if (ret == 1) | ||
| break; | ||
| if (ret < 0 && (errno != EAGAIN || errno != EINTR)) | ||
| if (ret < 0 && errno != EAGAIN && errno != EINTR) |
There was a problem hiding this comment.
Seems to be a buglet that predates quickjs-ng (also present in OG quickjs) but, on closer inspection, I think it should only be checking for EINTR, not EAGAIN.
The file descriptors are in blocking mode so EAGAIN is unexpected and should never happen. If it does, then a) something is wrong, and b) we'd be busy-looping.
Having said that, pre-existing condition. Doesn't need to hold up this PR.
There was a problem hiding this comment.
It is indeed a pre-existing condition and it seems to go back to 2020-09-06.
The condition simplifies as if (ret < 0) which ignores the potential need for restarting the write in case of a signal.
I wonder what it would take for clang to catch the constant expression (errno != EAGAIN || errno != EINTR))
The retry loop in js_waker_signal used `errno != EAGAIN || errno != EINTR`, which is always true (any single errno value is unequal to at least one of the two), so the loop bailed on the first short-write error and silently dropped the wakeup. Replace with `&&` so the loop only exits on a fatal errno. No test: js_waker_signal's transient-error path is reached only when the pipe is full or the syscall is interrupted by a signal, neither of which is reliably reproducible in a portable unit test. Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 14892f8)
4 participants
The retry loop in js_waker_signal used
errno != EAGAIN || errno != EINTR, which is always true (any single errno value is unequal to at least one of the two), so the loop bailed on the first short-write error and silently dropped the wakeup. Replace with&&so the loop only exits on a fatal errno.No test: js_waker_signal's transient-error path is reached only when the pipe is full or the syscall is interrupted by a signal, neither of which is reliably reproducible in a portable unit test.