From f05a040be65b7495855441fb13175841710b12e9 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Wed, 9 Nov 2022 13:12:48 +0100 Subject: [PATCH] Make `track` functions return correctly Previously we always returned `true`. Now we return what the node actually returns. Signed-off-by: Alexis Sellier --- Cargo.lock | 1 + radicle-node/Cargo.toml | 4 ++ radicle-node/src/client/handle.rs | 16 +++--- radicle-node/src/control.rs | 40 +++++++++----- radicle-node/src/lib.rs | 2 +- radicle-node/src/service/reactor.rs | 2 +- radicle-node/src/test.rs | 15 +++--- radicle-node/src/test/handle.rs | 14 ++--- radicle-node/src/test/simulator.rs | 3 -- radicle-node/src/{test => }/tests.rs | 0 radicle/Cargo.toml | 5 ++ radicle/src/node.rs | 81 ++++++++++++++++++++++++---- 12 files changed, 133 insertions(+), 50 deletions(-) rename radicle-node/src/{test => }/tests.rs (100%) diff --git a/Cargo.lock b/Cargo.lock index 89164333..fbcca0e7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1481,6 +1481,7 @@ dependencies = [ "radicle-cob", "radicle-crypto", "radicle-git-ext", + "radicle-node", "radicle-ssh", "serde", "serde_json", diff --git a/radicle-node/Cargo.toml b/radicle-node/Cargo.toml index 1a9463fe..faf32cbd 100644 --- a/radicle-node/Cargo.toml +++ b/radicle-node/Cargo.toml @@ -5,6 +5,9 @@ version = "0.2.0" authors = ["Alexis Sellier "] edition = "2021" +[features] +test = ["radicle/test", "radicle-crypto/test", "quickcheck"] + [dependencies] anyhow = { version = "1" } bloomy = { version = "1.2" } @@ -19,6 +22,7 @@ log = { version = "0.4.17", features = ["std"] } nakamoto-net = { version = "0.3.0" } nakamoto-net-poll = { version = "0.3.0" } nonempty = { version = "0.8.0", features = ["serialize"] } +quickcheck = { version = "1", default-features = false, optional = true } sqlite = { version = "0.28.1" } sqlite3-src = { version = "0.4.0", features = ["bundled"] } # Ensures static linking scrypt = { version = "0.10.0", default-features = false } diff --git a/radicle-node/src/client/handle.rs b/radicle-node/src/client/handle.rs index 2e276fe5..de6e1670 100644 --- a/radicle-node/src/client/handle.rs +++ b/radicle-node/src/client/handle.rs @@ -60,25 +60,25 @@ impl traits::Handle for Handle { self.listening.recv().map_err(Error::from) } - fn fetch(&self, id: Id) -> Result { + fn fetch(&mut self, id: Id) -> Result { let (sender, receiver) = chan::bounded(1); self.commands.send(service::Command::Fetch(id, sender))?; receiver.recv().map_err(Error::from) } - fn track(&self, id: Id) -> Result { + fn track(&mut self, id: Id) -> Result { let (sender, receiver) = chan::bounded(1); self.commands.send(service::Command::Track(id, sender))?; receiver.recv().map_err(Error::from) } - fn untrack(&self, id: Id) -> Result { + fn untrack(&mut self, id: Id) -> Result { let (sender, receiver) = chan::bounded(1); self.commands.send(service::Command::Untrack(id, sender))?; receiver.recv().map_err(Error::from) } - fn announce_refs(&self, id: Id) -> Result<(), Error> { + fn announce_refs(&mut self, id: Id) -> Result<(), Error> { self.command(service::Command::AnnounceRefs(id)) } @@ -143,14 +143,14 @@ pub mod traits { /// Wait for the node's listening socket to be bound. fn listening(&self) -> Result; /// Retrieve or update the project from network. - fn fetch(&self, id: Id) -> Result; + fn fetch(&mut self, id: Id) -> Result; /// Start tracking the given project. Doesn't do anything if the project is already /// tracked. - fn track(&self, id: Id) -> Result; + fn track(&mut self, id: Id) -> Result; /// Untrack the given project and delete it from storage. - fn untrack(&self, id: Id) -> Result; + fn untrack(&mut self, id: Id) -> Result; /// Notify the client that a project has been updated. - fn announce_refs(&self, id: Id) -> Result<(), Error>; + fn announce_refs(&mut self, id: Id) -> Result<(), Error>; /// Send a command to the command channel, and wake up the event loop. fn command(&self, cmd: service::Command) -> Result<(), Error>; /// Ask the client to shutdown. diff --git a/radicle-node/src/control.rs b/radicle-node/src/control.rs index 819509ce..8163798f 100644 --- a/radicle-node/src/control.rs +++ b/radicle-node/src/control.rs @@ -10,6 +10,7 @@ use std::{fs, io, net}; use crate::client; use crate::client::handle::traits::Handle; use crate::identity::Id; +use crate::node; use crate::service::FetchLookup; use crate::service::FetchResult; @@ -22,7 +23,7 @@ pub enum Error { } /// Listen for commands on the control socket, and process them. -pub fn listen, H: Handle>(path: P, handle: H) -> Result<(), Error> { +pub fn listen, H: Handle>(path: P, mut handle: H) -> Result<(), Error> { // Remove the socket file on startup before rebinding. fs::remove_file(&path).ok(); fs::create_dir_all( @@ -38,7 +39,7 @@ pub fn listen, H: Handle>(path: P, handle: H) -> Result<(), Error for incoming in listener.incoming() { match incoming { Ok(mut stream) => { - if let Err(e) = drain(&stream, &handle) { + if let Err(e) = drain(&stream, &mut handle) { log::error!("Received {} on control socket", e); writeln!(stream, "error: {}", e).ok(); @@ -68,8 +69,9 @@ enum DrainError { Io(#[from] io::Error), } -fn drain(stream: &UnixStream, handle: &H) -> Result<(), DrainError> { +fn drain(stream: &UnixStream, handle: &mut H) -> Result<(), DrainError> { let mut reader = BufReader::new(stream); + let mut writer = LineWriter::new(stream); // TODO: refactor to include helper for line in reader.by_ref().lines().flatten() { @@ -83,8 +85,17 @@ fn drain(stream: &UnixStream, handle: &H) -> Result<(), DrainError> { } Some(("track", arg)) => { if let Ok(id) = arg.parse() { - if let Err(e) = handle.track(id) { - return Err(DrainError::Client(e)); + match handle.track(id) { + Ok(updated) => { + if updated { + writeln!(writer, "{}", node::RESPONSE_OK)?; + } else { + writeln!(writer, "{}", node::RESPONSE_NOOP)?; + } + } + Err(e) => { + return Err(DrainError::Client(e)); + } } } else { return Err(DrainError::InvalidCommandArg(arg.to_owned())); @@ -92,8 +103,17 @@ fn drain(stream: &UnixStream, handle: &H) -> Result<(), DrainError> { } Some(("untrack", arg)) => { if let Ok(id) = arg.parse() { - if let Err(e) = handle.untrack(id) { - return Err(DrainError::Client(e)); + match handle.untrack(id) { + Ok(updated) => { + if updated { + writeln!(writer, "{}", node::RESPONSE_OK)?; + } else { + writeln!(writer, "{}", node::RESPONSE_NOOP)?; + } + } + Err(e) => { + return Err(DrainError::Client(e)); + } } } else { return Err(DrainError::InvalidCommandArg(arg.to_owned())); @@ -114,8 +134,6 @@ fn drain(stream: &UnixStream, handle: &H) -> Result<(), DrainError> { None => match line.as_str() { "routing" => match handle.routing() { Ok(c) => { - let mut writer = LineWriter::new(stream); - for (id, seed) in c.iter() { writeln!(writer, "{id} {seed}",)?; } @@ -124,8 +142,6 @@ fn drain(stream: &UnixStream, handle: &H) -> Result<(), DrainError> { }, "inventory" => match handle.inventory() { Ok(c) => { - let mut writer = LineWriter::new(stream); - for id in c.iter() { writeln!(writer, "{id}")?; } @@ -141,7 +157,7 @@ fn drain(stream: &UnixStream, handle: &H) -> Result<(), DrainError> { Ok(()) } -fn fetch(id: Id, mut writer: W, handle: &H) -> Result<(), DrainError> { +fn fetch(id: Id, mut writer: W, handle: &mut H) -> Result<(), DrainError> { match handle.fetch(id) { Err(e) => { return Err(DrainError::Client(e)); diff --git a/radicle-node/src/lib.rs b/radicle-node/src/lib.rs index ac0e4982..2764840f 100644 --- a/radicle-node/src/lib.rs +++ b/radicle-node/src/lib.rs @@ -6,7 +6,7 @@ pub mod decoder; pub mod logger; pub mod service; pub mod sql; -#[cfg(test)] +#[cfg(any(test, feature = "test"))] pub mod test; pub mod transport; pub mod wire; diff --git a/radicle-node/src/service/reactor.rs b/radicle-node/src/service/reactor.rs index 25f098e9..9880a1c0 100644 --- a/radicle-node/src/service/reactor.rs +++ b/radicle-node/src/service/reactor.rs @@ -104,7 +104,7 @@ impl Reactor { } } - #[cfg(test)] + #[cfg(any(test, feature = "test"))] pub(crate) fn outbox(&mut self) -> &mut VecDeque { &mut self.io } diff --git a/radicle-node/src/test.rs b/radicle-node/src/test.rs index 8d7a9996..8660381a 100644 --- a/radicle-node/src/test.rs +++ b/radicle-node/src/test.rs @@ -1,12 +1,9 @@ -pub(crate) mod arbitrary; -pub(crate) mod gossip; -pub(crate) mod handle; -pub(crate) mod logger; -pub(crate) mod peer; -pub(crate) mod simulator; -pub(crate) mod tests; +pub mod arbitrary; +pub mod gossip; +pub mod handle; +pub mod logger; +pub mod peer; +pub mod simulator; -#[cfg(test)] pub use radicle::assert_matches; -#[cfg(test)] pub use radicle::test::*; diff --git a/radicle-node/src/test/handle.rs b/radicle-node/src/test/handle.rs index 5606b868..fcc8688b 100644 --- a/radicle-node/src/test/handle.rs +++ b/radicle-node/src/test/handle.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::sync::{Arc, Mutex}; use crossbeam_channel as chan; @@ -11,6 +12,7 @@ use crate::service::FetchLookup; #[derive(Default, Clone)] pub struct Handle { pub updates: Arc>>, + pub tracking: HashSet, } impl traits::Handle for Handle { @@ -18,19 +20,19 @@ impl traits::Handle for Handle { unimplemented!() } - fn fetch(&self, _id: Id) -> Result { + fn fetch(&mut self, _id: Id) -> Result { Ok(FetchLookup::NotFound) } - fn track(&self, _id: Id) -> Result { - Ok(true) + fn track(&mut self, id: Id) -> Result { + Ok(self.tracking.insert(id)) } - fn untrack(&self, _id: Id) -> Result { - Ok(true) + fn untrack(&mut self, id: Id) -> Result { + Ok(self.tracking.remove(&id)) } - fn announce_refs(&self, id: Id) -> Result<(), Error> { + fn announce_refs(&mut self, id: Id) -> Result<(), Error> { self.updates.lock().unwrap().push(id); Ok(()) diff --git a/radicle-node/src/test/simulator.rs b/radicle-node/src/test/simulator.rs index 159c470c..6a3b0452 100644 --- a/radicle-node/src/test/simulator.rs +++ b/radicle-node/src/test/simulator.rs @@ -2,9 +2,6 @@ #![allow(clippy::collapsible_if)] #![allow(dead_code)] -#[cfg(feature = "quickcheck")] -pub mod arbitrary; - use std::collections::{BTreeMap, BTreeSet, VecDeque}; use std::marker::PhantomData; use std::ops::{Deref, DerefMut, Range}; diff --git a/radicle-node/src/test/tests.rs b/radicle-node/src/tests.rs similarity index 100% rename from radicle-node/src/test/tests.rs rename to radicle-node/src/tests.rs diff --git a/radicle/Cargo.toml b/radicle/Cargo.toml index a0eac4ce..b86b517f 100644 --- a/radicle/Cargo.toml +++ b/radicle/Cargo.toml @@ -64,3 +64,8 @@ quickcheck = { version = "1", default-features = false } path = "../radicle-crypto" version = "0" features = ["test"] + +[dev-dependencies.radicle-node] +path = "../radicle-node" +version = "0" +features = ["test"] diff --git a/radicle/src/node.rs b/radicle/src/node.rs index b061ec2f..9c9b31f7 100644 --- a/radicle/src/node.rs +++ b/radicle/src/node.rs @@ -13,11 +13,19 @@ pub use features::Features; /// Default name for control socket file. pub const DEFAULT_SOCKET_NAME: &str = "radicle.sock"; +/// Response on node socket indicating that a command was carried out successfully. +pub const RESPONSE_OK: &str = "ok"; +/// Response on node socket indicating that a command had no effect. +pub const RESPONSE_NOOP: &str = "noop"; #[derive(thiserror::Error, Debug)] pub enum Error { #[error("failed to connect to node: {0}")] Connect(#[from] io::Error), + #[error("received invalid response for `{cmd}` command: '{response}'")] + InvalidResponse { cmd: &'static str, response: String }, + #[error("received empty response for `{cmd}` command")] + EmptyResponse { cmd: &'static str }, } pub trait Handle { @@ -67,31 +75,49 @@ impl Handle for Node { fn fetch(&self, id: &Id) -> Result<(), Error> { for line in self.call("fetch", id)? { let line = line?; - log::info!("node: {}", line); + log::debug!("node: {}", line); } Ok(()) } fn track(&self, id: &Id) -> Result { - for line in self.call("track", id)? { - let line = line?; - log::info!("node: {}", line); + let mut line = self.call("track", id)?; + let line = line.next().ok_or(Error::EmptyResponse { cmd: "track" })??; + + log::debug!("node: {}", line); + + match line.as_str() { + RESPONSE_OK => Ok(true), + RESPONSE_NOOP => Ok(false), + _ => Err(Error::InvalidResponse { + cmd: "track", + response: line, + }), } - Ok(true) } fn untrack(&self, id: &Id) -> Result { - for line in self.call("untrack", id)? { - let line = line?; - log::info!("node: {}", line); + let mut line = self.call("untrack", id)?; + let line = line + .next() + .ok_or(Error::EmptyResponse { cmd: "untrack" })??; + + log::debug!("node: {}", line); + + match line.as_str() { + RESPONSE_OK => Ok(true), + RESPONSE_NOOP => Ok(false), + _ => Err(Error::InvalidResponse { + cmd: "untrack", + response: line, + }), } - Ok(true) } fn announce_refs(&self, id: &Id) -> Result<(), Error> { for line in self.call("announce-refs", id)? { let line = line?; - log::info!("node: {}", line); + log::debug!("node: {}", line); } Ok(()) } @@ -105,3 +131,38 @@ impl Handle for Node { pub fn connect>(path: P) -> Result { Node::connect(path) } + +#[cfg(test)] +mod tests { + use std::thread; + + use super::*; + use crate::test; + + #[test] + fn test_track_untrack() { + let tmp = tempfile::tempdir().unwrap(); + let socket = tmp.path().join("node.sock"); + let proj = test::arbitrary::gen::(1); + + thread::spawn({ + use radicle_node as node; + + let socket = socket.clone(); + let handle = node::test::handle::Handle::default(); + + move || node::control::listen(socket, handle) + }); + + let handle = loop { + if let Ok(conn) = Node::connect(&socket) { + break conn; + } + }; + + assert!(handle.track(&proj).unwrap()); + assert!(!handle.track(&proj).unwrap()); + assert!(handle.untrack(&proj).unwrap()); + assert!(!handle.untrack(&proj).unwrap()); + } +}