Skip to content

Commit

Permalink
fix(signals): loop sigtimedwait() on SIGWINCH (#32)
Browse files Browse the repository at this point in the history
Our signal-handling looks a little different than resty-cli, which uses
sigaction() to register a signal handler function. rusty-cli uses
sigtimedwait() because the API and control flow is much more convenient.

Previously we were exiting our signal-handling phase after the first
signal was caught, forwarding it to the child process and then
wait4()-ing for it to exit. This is mostly correct, except in the case
of SIGWINCH, which isn't a "you should probably stop now/soon" signal.

To fix this we loop over sigtimedwait() until we get a non-SIGWINCH
signal.

I've also attempted to fix a race condition wherein we could hang if the
child process exited before we could set up our signal blocking mask and
await signal delivery by explicitly checking the child process on each
iteration _before_ blocking for signal delivery.
  • Loading branch information
flrgh committed Dec 21, 2023
1 parent b744e42 commit dd0cd5d
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 61 deletions.
3 changes: 3 additions & 0 deletions .github/workflows/test-compat.yml
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,6 @@ jobs:

- name: lua script argv translation
run: ./tests/lua-arg.sh

- name: signal-handling
run: ./tests/sigwinch.sh
33 changes: 22 additions & 11 deletions src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ fn block_wait(mut proc: Child) -> i32 {
}

pub(crate) fn run(mut cmd: Command) -> i32 {
let proc = match cmd.spawn() {
let mask = SigSet::from_iter(SIGNALS);

let mut proc = match cmd.spawn() {
Ok(child) => child,
Err(e) => {
let prog = cmd.get_program().to_string_lossy();
Expand All @@ -55,21 +57,30 @@ pub(crate) fn run(mut cmd: Command) -> i32 {
}
};

let caught = {
let mask = SigSet::from_iter(SIGNALS);
let old_mask = mask
.thread_swap_mask(SIG_BLOCK)
.expect("failed to block signals");
let old_mask = mask
.thread_swap_mask(SIG_BLOCK)
.expect("failed to block signals");

let signal = mask.wait().expect("failed to wait for signal");
let caught = loop {
// we need to check if the child process has exited some time between
// when we set up our signal mask and now
if let Ok(Some(_)) = proc.try_wait() {
break SIGCHLD;
}

old_mask
.thread_set_mask()
.expect("failed to restore thread signal mask");
let signal = mask.wait().expect("failed to wait for signal");

signal
if signal == SIGWINCH {
send_signal(&proc, SIGWINCH);
} else {
break signal;
}
};

old_mask
.thread_set_mask()
.expect("failed to restore thread signal mask");

match caught {
SIGCHLD => block_wait(proc),
SIGINT => {
Expand Down
50 changes: 0 additions & 50 deletions tests/signals.sh

This file was deleted.

74 changes: 74 additions & 0 deletions tests/sigwinch.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#!/usr/bin/env bash

set -euo pipefail

readonly RUSTY=./target/debug/rusty-cli
readonly RESTY=./resty-cli/bin/resty

test_it() {
local -r bin=$1
echo "TEST: $bin"

local tmp; tmp=$(mktemp)

"$bin" \
-e "local fh = io.open('$tmp', 'w+'); fh:write(ngx.config.prefix()); fh:flush(); fh:close()" \
-e 'ngx.sleep(60)' \
&

local pid=$!

while [[ ! -s "$tmp" ]]; do
kill -0 "$pid" || {
echo "fatal: $pid died while we were waiting"
exit 1
}
sleep 1
done

local prefix; prefix=$(<"$tmp")
if [[ -d $prefix ]]; then
echo "prefix dir: $prefix"
else
echo "fatal: nginx prefix dir ($prefix) does not exist at startup"
exit 1
fi

echo -n "sending WINCH..."
for _ in {1..20}; do
echo -n .
kill -WINCH "$pid"
sleep 0.1
done

echo

if [[ ! -d $prefix ]]; then
echo "fatal: nginx prefix dir ($prefix) does not exist after WINCH"
fi

echo "sending INT..."
kill -INT "$pid"

set +e
wait "$pid"
ec=$?
set -e

echo "$bin has exited with code: $ec"

if [[ -d $prefix ]]; then
echo "FAILED: nginx prefix dir ($prefix) still exists after stopping"
exit 1
fi

if (( ec != 130 )); then
echo "FAILED: unexpected exit code ($ec)"
exit 1
fi

echo "OK"
}

test_it "$RESTY"
test_it "$RUSTY"

0 comments on commit dd0cd5d

Please sign in to comment.