[plebhash]

follow-up to #689

implements some suggestions from #914, except the tProxy sv1 draining part

---

IIUC the stop_signal on docker-compose.yml was a workaround for the lack of proper SIGTERM handling, so it's no longer needed (cc @lucasbalieiro )

[lucasbalieiro]

IIUC the stop_signal on docker-compose.yml was a workaround for the lack of proper SIGTERM handling, so it's no longer needed (cc @lucasbalieiro )

yes, exactly; historical context : #299 (comment)

--
tACK the docker deployment:
running the changes locally the containers now exit with a graceful shutdown (code 0) without the stop_signal

Container tproxy_sv2 Stopped 
tproxy_sv2 exited with code 0
Container jd_client_sv2 Stopped 
jd_client_sv2 exited with code 0
Container pool_sv2 Stopped 
pool_sv2 exited with code 0

[bit-aloo]

Solid improvement on signal handling. Few suggestions

[bit-aloo]

Few suggestions, once we do that. Its good to go. Thanks.

[bit-aloo]

Instead of vanilla strs and error codes, lets have this:

/// A signal asking the process to stop.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum StopSignal {
    /// `SIGINT`, sent by an interactive terminal on Ctrl+C.
    Interrupt,
    /// `SIGTERM`, sent by service managers and container runtimes.
    Terminate,
}

impl StopSignal {
    /// The exit status conventionally associated with this signal.
    ///
    /// That status is `128 + the signal number` (`SIGINT` is 2, `SIGTERM` is 15), which is what a
    /// shell or a container runtime reports when a signal terminates a process. Exiting with it
    /// is not the same outcome as being terminated by the signal, which a supervisor can tell
    /// apart, but it is the number tooling already reads as "stopped by an operator".
    pub fn exit_code(self) -> i32 {
        match self {
            Self::Interrupt => 130,
            Self::Terminate => 143,
        }
    }
}

impl fmt::Display for StopSignal {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.write_str(match self {
            Self::Interrupt => "SIGINT",
            Self::Terminate => "SIGTERM",
        })
    }
}

[plebhash]

done

[bit-aloo]

Lets have a new API here:

    pub async fn run_until_stopped(mut self, graceful: impl Future<Output = ()>) {
        let signal = self.wait().await;
        info!("{signal} received — initiating graceful shutdown...");
        tokio::select! {
            _ = graceful => {}
            signal = self.wait() => {
                warn!("{signal} received again — abandoning graceful shutdown");
                std::process::exit(signal.exit_code());
            }
        }
    }

This we can use for all apps startup, kinda similar code everywhere.

[plebhash]

I'm "ponytail" skeptic against the suggestion and I don't want to use this PR as an opportunity to introduce a new API pattern just because it feels cool

there's already repetition on main.rs of all apps (all benign IMO), and if we want to address that we should do it in a consistent way

[bit-aloo]

ACK. Thanks