refactor: Abstract SocketDescriptor writing queue

To enable better testing, put the code to enqueue data and flush it to
a SocketDescriptor behind an object.

This patch starts to implement traits for the various separate interface
that each object needs. The idea is the consumer of an interface defines
it and the dependency objects implement it. This will be useful when
creating test doubles for unit tests and helps make the dependencies
of each function more clear.

Additional uses of trait-based contracts will be more clear in the next
patches that start to add more testing.

Author: julianknutsen

lightning/src/ln/peers/handler.rs

@@ -20,7 +20,7 @@ use util::events::{MessageSendEvent, MessageSendEventsProvider};
 use util::logger::Logger;
 use routing::network_graph::NetGraphMsgHandler;
 
-use std::collections::{HashMap,hash_map,HashSet,LinkedList};
+use std::collections::{HashMap,hash_map,HashSet};
 use std::sync::{Arc, Mutex};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::{cmp,error,hash,fmt};
@@ -31,6 +31,9 @@ use bitcoin::hashes::sha256::HashEngine as Sha256Engine;
 use bitcoin::hashes::{HashEngine, Hash};
 use ln::peers::handshake::PeerHandshake;
 use ln::peers::conduit::Conduit;
+use ln::peers::outbound_buffer::OutboundBuffer;
+
+const MSG_BUFF_SIZE: usize = 10;
 
 /// Provides references to trait impls which handle different types of messages.
 pub struct MessageHandler<CM: Deref, RM: Deref> where
@@ -111,6 +114,64 @@ enum InitSyncTracker{
 	NodesSyncing(PublicKey),
 }
 
+/// Trait representing a container that allows enqueuing of Vec<[u8]>
+pub(super) trait Queueable {
+	/// Enqueue item to the queue
+	fn push_back(&mut self, item: Vec<u8>);
+
+	/// Returns true if the queue is empty
+	fn is_empty(&self) -> bool;
+
+	/// Returns the amount of available space in queue
+	fn queue_space(&self) -> usize;
+}
+
+/// Implement &mut Queueable passthroughs
+impl<'a, T> Queueable for &'a mut T where
+	T: Queueable {
+	fn push_back(&mut self, item: Vec<u8>) {
+		T::push_back(self, item)
+	}
+
+	fn is_empty(&self) -> bool {
+		T::is_empty(self)
+	}
+
+	fn queue_space(&self) -> usize {
+		T::queue_space(self)
+	}
+}
+
+/// Trait representing a container that can try to flush data through a SocketDescriptor
+pub(super) trait Flushable {
+	/// Write previously enqueued data to the SocketDescriptor. A return of false indicates the
+	/// underlying SocketDescriptor could not fulfill the send_data() call and the blocked state
+	/// has been set. Use unblock() when the SocketDescriptor may have more room.
+	fn try_flush_one(&mut self, descriptor: &mut impl SocketDescriptor) -> bool;
+
+	/// Clear the blocked state caused when a previous write failed
+	fn unblock(&mut self);
+
+	/// Check if the container is in a blocked state
+	fn is_blocked(&self) -> bool;
+}
+
+/// Implement &mut Flushable passthroughs
+impl<'a, T> Flushable for &'a mut T where
+	T: Flushable {
+	fn try_flush_one(&mut self, descriptor: &mut impl SocketDescriptor) -> bool {
+		T::try_flush_one(self, descriptor)
+	}
+
+	fn unblock(&mut self) {
+		T::unblock(self)
+	}
+
+	fn is_blocked(&self) -> bool {
+		T::is_blocked(self)
+	}
+}
+
 enum PeerState {
 	Authenticating(PeerHandshake),
 	Connected(Conduit),
@@ -130,7 +191,7 @@ impl PeerState {
 		}
 	}
 
-	fn process_peer_data(&mut self, data: &[u8], mutable_response_buffer: &mut LinkedList<Vec<u8>>) -> PeerDataProcessingDecision {
+	fn process_peer_data(&mut self, data: &[u8], pending_outbound_buffer: &mut impl Queueable) -> PeerDataProcessingDecision {
 		let (new_state_opt, decision) = match self {
 			&mut PeerState::Authenticating(ref mut handshake) => {
 				match handshake.process_act(data) {
@@ -144,7 +205,7 @@ impl PeerState {
 
 						// Any response generated by the handshake sequence is put into the response buffer
 						if let Some(response_vec) = response_vec_option {
-							mutable_response_buffer.push_back(response_vec);
+							pending_outbound_buffer.push_back(response_vec);
 						}
 
 						// if process_act() returns the conduit and remote static public key (node id)
@@ -177,9 +238,7 @@ struct Peer {
 	their_node_id: Option<PublicKey>,
 	their_features: Option<InitFeatures>,
 
-	pending_outbound_buffer: LinkedList<Vec<u8>>,
-	pending_outbound_buffer_first_msg_offset: usize,
-	awaiting_write_event: bool,
+	pending_outbound_buffer: OutboundBuffer,
 
 	sync_status: InitSyncTracker,
 
@@ -366,9 +425,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 			their_node_id: Some(their_node_id.clone()),
 			their_features: None,
 
-			pending_outbound_buffer: LinkedList::new(),
-			pending_outbound_buffer_first_msg_offset: 0,
-			awaiting_write_event: false,
+			pending_outbound_buffer: OutboundBuffer::new(MSG_BUFF_SIZE),
 
 			sync_status: InitSyncTracker::NoSyncRequested,
 
@@ -398,9 +455,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 			their_node_id: None,
 			their_features: None,
 
-			pending_outbound_buffer: LinkedList::new(),
-			pending_outbound_buffer_first_msg_offset: 0,
-			awaiting_write_event: false,
+			pending_outbound_buffer: OutboundBuffer::new(MSG_BUFF_SIZE),
 
 			sync_status: InitSyncTracker::NoSyncRequested,
 
@@ -423,13 +478,14 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 				}
 			}
 		}
-		const MSG_BUFF_SIZE: usize = 10;
-		while !peer.awaiting_write_event {
-			if peer.pending_outbound_buffer.len() < MSG_BUFF_SIZE {
+
+		while !peer.pending_outbound_buffer.is_blocked() {
+			let queue_space = peer.pending_outbound_buffer.queue_space();
+			if queue_space > 0 {
 				match peer.sync_status {
 					InitSyncTracker::NoSyncRequested => {},
 					InitSyncTracker::ChannelsSyncing(c) if c < 0xffff_ffff_ffff_ffff => {
-						let steps = ((MSG_BUFF_SIZE - peer.pending_outbound_buffer.len() + 2) / 3) as u8;
+						let steps = ((queue_space + 2) / 3) as u8;
 						let all_messages = self.message_handler.route_handler.get_next_channel_announcements(c, steps);
 						for &(ref announce, ref update_a_option, ref update_b_option) in all_messages.iter() {
 							encode_and_send_msg!(announce);
@@ -446,7 +502,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 						}
 					},
 					InitSyncTracker::ChannelsSyncing(c) if c == 0xffff_ffff_ffff_ffff => {
-						let steps = (MSG_BUFF_SIZE - peer.pending_outbound_buffer.len()) as u8;
+						let steps = queue_space as u8;
 						let all_messages = self.message_handler.route_handler.get_next_node_announcements(None, steps);
 						for msg in all_messages.iter() {
 							encode_and_send_msg!(msg);
@@ -458,7 +514,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 					},
 					InitSyncTracker::ChannelsSyncing(_) => unreachable!(),
 					InitSyncTracker::NodesSyncing(key) => {
-						let steps = (MSG_BUFF_SIZE - peer.pending_outbound_buffer.len()) as u8;
+						let steps = queue_space as u8;
 						let all_messages = self.message_handler.route_handler.get_next_node_announcements(Some(&key), steps);
 						for msg in all_messages.iter() {
 							encode_and_send_msg!(msg);
@@ -471,23 +527,12 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 				}
 			}
 
-			if {
-				let next_buff = match peer.pending_outbound_buffer.front() {
-					None => return,
-					Some(buff) => buff,
-				};
-
-				let should_be_reading = peer.pending_outbound_buffer.len() < MSG_BUFF_SIZE;
-				let pending = &next_buff[peer.pending_outbound_buffer_first_msg_offset..];
-				let data_sent = descriptor.send_data(pending, should_be_reading);
-				peer.pending_outbound_buffer_first_msg_offset += data_sent;
-				if peer.pending_outbound_buffer_first_msg_offset == next_buff.len() { true } else { false }
-			} {
-				peer.pending_outbound_buffer_first_msg_offset = 0;
-				peer.pending_outbound_buffer.pop_front();
-			} else {
-				peer.awaiting_write_event = true;
+			// No messages to send
+			if peer.pending_outbound_buffer.is_empty() {
+				break;
 			}
+
+			peer.pending_outbound_buffer.try_flush_one(descriptor);
 		}
 	}
 
@@ -506,7 +551,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 		match peers.peers.get_mut(descriptor) {
 			None => panic!("Descriptor for write_event is not already known to PeerManager"),
 			Some(peer) => {
-				peer.awaiting_write_event = false;
+				peer.pending_outbound_buffer.unblock();
 				self.do_attempt_write_data(descriptor, peer);
 			}
 		};
@@ -684,7 +729,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 
 					self.do_attempt_write_data(peer_descriptor, peer);
 
-					peer.pending_outbound_buffer.len() > 10 // pause_read
+					peer.pending_outbound_buffer.queue_space() == 0 // pause_read
 				}
 			};
 

lightning/src/ln/peers/mod.rs

@@ -6,6 +6,7 @@
 mod chacha;
 pub mod handler;
 mod hkdf5869rfc;
+mod outbound_buffer;
 
 #[cfg(feature = "fuzztarget")]
 pub mod conduit;