Skip to content

Commit

Permalink
Removed signals hook in socket. (#1201)
Browse files Browse the repository at this point in the history
The signals hook caused Node to not respond to ^C and similar signals, since Node yields to existing signals hooks.
Tested Python, Node, C#, & Java benchmarks - all closed with ^C after this change.
  • Loading branch information
nihohit authored Apr 1, 2024
1 parent 70c0355 commit c9537db
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 36 deletions.
4 changes: 1 addition & 3 deletions glide-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ authors = ["Amazon Web Services"]
bytes = { version = "^1.3", optional = true }
futures = "^0.3"
redis = { path = "../submodules/redis-rs/redis", features = ["aio", "tokio-comp", "tokio-rustls-comp", "connection-manager","cluster", "cluster-async"] }
signal-hook = { version = "^0.3", optional = true }
signal-hook-tokio = {version = "^0.3", features = ["futures-v0_3"], optional = true }
tokio = { version = "1", features = ["macros", "time"] }
logger_core = {path = "../logger_core"}
dispose = "0.5.0"
Expand All @@ -30,7 +28,7 @@ arcstr = "1.1.5"
sha1_smol = "1.0.0"

[features]
socket-layer = ["directories", "integer-encoding", "num_cpus", "signal-hook", "signal-hook-tokio", "protobuf", "tokio-util", "bytes", "rand"]
socket-layer = ["directories", "integer-encoding", "num_cpus", "protobuf", "tokio-util", "bytes", "rand"]

[dev-dependencies]
rsevents = "0.3.1"
Expand Down
44 changes: 11 additions & 33 deletions glide-core/src/socket_listener.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use crate::response::Response;
use crate::retry_strategies::get_fixed_interval_backoff;
use directories::BaseDirs;
use dispose::{Disposable, Dispose};
use futures::stream::StreamExt;
use logger_core::{log_debug, log_error, log_info, log_trace, log_warn};
use protobuf::Message;
use redis::cluster_routing::{
Expand All @@ -23,8 +22,6 @@ use redis::cluster_routing::{
use redis::cluster_routing::{ResponsePolicy, Routable};
use redis::RedisError;
use redis::{cmd, Cmd, Value};
use signal_hook::consts::signal::*;
use signal_hook_tokio::Signals;
use std::cell::Cell;
use std::rc::Rc;
use std::{env, str};
Expand Down Expand Up @@ -781,19 +778,17 @@ impl SocketListener {
init_callback(Ok(self.socket_path.clone()));
let local_set_pool = LocalPoolHandle::new(num_cpus::get());
loop {
tokio::select! {
listen_v = listener.accept() => {
if let Ok((stream, _addr)) = listen_v {
// New client
local_set_pool.spawn_pinned(move || {
listen_on_client_stream(stream)
});
} else if listen_v.is_err() {
return
}
},
// Interrupt was received, close the socket
_ = handle_signals() => return
match listener.accept().await {
Ok((stream, _addr)) => {
local_set_pool.spawn_pinned(move || listen_on_client_stream(stream));
}
Err(err) => {
log_debug(
"listen_on_socket",
format!("Socket closed with error: `{err}`"),
);
return;
}
}
}
}
Expand Down Expand Up @@ -879,23 +874,6 @@ pub fn get_socket_path() -> String {
get_socket_path_from_name(socket_name)
}

async fn handle_signals() {
// Handle Unix signals
let mut signals =
Signals::new([SIGTERM, SIGQUIT, SIGINT, SIGHUP]).expect("Failed creating signals");
loop {
if let Some(signal) = signals.next().await {
match signal {
SIGTERM | SIGQUIT | SIGINT | SIGHUP => {
log_info("connection", format!("Signal {signal:?} received"));
return;
}
_ => continue,
}
}
}
}

/// This function is exposed only for the sake of testing with a nonstandard `socket_path`.
/// Avoid using this function, unless you explicitly want to test the behavior of the listener
/// without using the sockets used by other tests.
Expand Down

0 comments on commit c9537db

Please sign in to comment.