Connection Manager enhancements #371

RyanLassigBanks · 2019-12-05T15:27:49Z

Replace crossbeam with standard library channels
Remove locks around connection state
Move the heartbeat monitor into its own module
Add remove connection to connection manager

libsplinter/src/network/connection_manager/heartbeat_monitor.rs

libsplinter/src/network/connection_manager/messages.rs

libsplinter/src/network/connection_manager/mod.rs

Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Heartbeat monitor and connection manager no longer share ConnectionState. Heartbeat messages are now sent from the connection manager thread and requested by the heartbeat monitor instead of the heartbeat monitor sending them directly. Signed-off-by: Ryan Banks <rbanks@bitwise.io>

libsplinter/src/network/connection_manager/mod.rs

vaporos · 2019-12-12T19:03:57Z

libsplinter/src/network/connection_manager/mod.rs

-        ConnectionManagerError::HeartbeatError("cannot create NetworkHeartbeat message".to_string())
-    })?;
+fn create_heartbeat() -> Vec<u8> {
+    let heartbeat = NetworkHeartbeat::new().write_to_bytes().unwrap();


Why is panicking here desirable?

I believe @peterschwarz mentioned this a few times, but write_to_bytes only fails if the system completely runs out of memory.

This deviates from the way we handle these errors everywhere else in splinter, so I'm not sure I 100% agree with this change from a consistency perspective, but I won't block the PR because of that. However, this should be changed to an expect to provide an error message.

Peter is OOO until Tuesday, and I'd like to try to get his merged before then.

ltseeley · 2019-12-13T16:00:21Z

libsplinter/src/network/connection_manager/mod.rs

@@ -62,7 +61,13 @@ impl ConnectionManager {

    pub fn start(&mut self) -> Result<Connector, ConnectionManagerError> {
        let (sender, recv) = sync_channel(CHANNEL_CAPACITY);
-        let mut state = self.connection_state.clone();
+        let mut state = if let Some(state) = self.connection_state.take() {


This if let ... else ... could be simplified by using self.connection_state.take().ok_or_else(|| Err(...))?

ltseeley · 2019-12-13T16:06:49Z

libsplinter/src/network/connection_manager/mod.rs

-        ConnectionManagerError::HeartbeatError("cannot create NetworkHeartbeat message".to_string())
-    })?;
+fn create_heartbeat() -> Vec<u8> {
+    let heartbeat = NetworkHeartbeat::new().write_to_bytes().unwrap();


This deviates from the way we handle these errors everywhere else in splinter, so I'm not sure I 100% agree with this change from a consistency perspective, but I won't block the PR because of that. However, this should be changed to an expect to provide an error message.

ConnectionState needs to be passed to the connection_manager thread in `ConnectionManager::start`. ConnectionState can't be clonable because Transports aren't clonable, so in order to pass it to the thread it will be temporarily stored as an option. Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Fixing clippy errors that were introduced when the connection-manager feature was initially added Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Change test ports from 8080 to 3030 to avoid conflicts with other tests that use port 8080. Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Signed-off-by: Ryan Banks <rbanks@bitwise.io>

RyanLassigBanks requested review from agunde406, annechenette, davececchi, dplumb94, eloaverona, jsmitchell, ltseeley, peterschwarz, rbuysse, shannynalayna, vaporos and zapplebee as code owners December 5, 2019 15:27

RyanLassigBanks self-assigned this Dec 5, 2019

RyanLassigBanks force-pushed the peering-manager-mkIV branch from 2ab635e to 6982c2a Compare December 5, 2019 15:51

vaporos reviewed Dec 5, 2019

View reviewed changes

libsplinter/src/network/connection_manager/heartbeat_monitor.rs Outdated Show resolved Hide resolved

vaporos reviewed Dec 5, 2019

View reviewed changes

libsplinter/src/network/connection_manager/messages.rs Show resolved Hide resolved

agunde406 previously approved these changes Dec 6, 2019

View reviewed changes

RyanLassigBanks dismissed agunde406’s stale review via 1e891c3 December 6, 2019 22:16

RyanLassigBanks force-pushed the peering-manager-mkIV branch from 17937ca to 1e891c3 Compare December 6, 2019 22:16

agunde406 requested review from vaporos and agunde406 December 9, 2019 14:05

agunde406 suggested changes Dec 9, 2019

View reviewed changes

libsplinter/src/network/connection_manager/mod.rs Show resolved Hide resolved

RyanLassigBanks requested a review from agunde406 December 9, 2019 14:51

RyanLassigBanks force-pushed the peering-manager-mkIV branch from 1e891c3 to 3e06953 Compare December 9, 2019 14:56

agunde406 previously approved these changes Dec 9, 2019

View reviewed changes

peterschwarz previously requested changes Dec 9, 2019

View reviewed changes

libsplinter/src/network/connection_manager/mod.rs Outdated Show resolved Hide resolved

libsplinter/src/network/connection_manager/mod.rs Show resolved Hide resolved

libsplinter/src/network/connection_manager/mod.rs Show resolved Hide resolved

RyanLassigBanks added 3 commits December 12, 2019 10:42

Make Transport Send

81156af

Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Replace crossbeam with sync channels for std

dc33f9d

Signed-off-by: Ryan Banks <rbanks@bitwise.io>

RyanLassigBanks dismissed agunde406’s stale review via 5ec97b4 December 12, 2019 17:51

RyanLassigBanks force-pushed the peering-manager-mkIV branch from 3e06953 to 5ec97b4 Compare December 12, 2019 17:51

RyanLassigBanks requested review from peterschwarz and agunde406 December 12, 2019 17:51

vaporos reviewed Dec 12, 2019

View reviewed changes

RyanLassigBanks force-pushed the peering-manager-mkIV branch from 5ec97b4 to e4783fd Compare December 12, 2019 19:27

RyanLassigBanks requested a review from vaporos December 12, 2019 19:30

RyanLassigBanks force-pushed the peering-manager-mkIV branch from e4783fd to db3afc5 Compare December 12, 2019 20:21

ltseeley suggested changes Dec 13, 2019

View reviewed changes

RyanLassigBanks force-pushed the peering-manager-mkIV branch from db3afc5 to 5071d4e Compare December 13, 2019 16:19

RyanLassigBanks requested a review from ltseeley December 13, 2019 16:19

RyanLassigBanks force-pushed the peering-manager-mkIV branch 2 times, most recently from 46903d2 to 8f51057 Compare December 13, 2019 16:42

ltseeley previously approved these changes Dec 13, 2019

View reviewed changes

RyanLassigBanks added 6 commits December 13, 2019 11:22

Move Heartbeat Monitor into its own module

e4c5d79

Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Add remove connection to connection manager

45c0874

Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Fix clippy errors

8094981

Fixing clippy errors that were introduced when the connection-manager feature was initially added Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Change test ports

4a72786

Change test ports from 8080 to 3030 to avoid conflicts with other tests that use port 8080. Signed-off-by: Ryan Banks <rbanks@bitwise.io>

Rename heartbeat_monitor to pacemaker

1bd5356

Signed-off-by: Ryan Banks <rbanks@bitwise.io>

RyanLassigBanks dismissed ltseeley’s stale review via 1bd5356 December 13, 2019 17:22

RyanLassigBanks force-pushed the peering-manager-mkIV branch from f90530b to 1bd5356 Compare December 13, 2019 17:22

RyanLassigBanks requested a review from ltseeley December 13, 2019 17:22

vaporos approved these changes Dec 13, 2019

View reviewed changes

ltseeley approved these changes Dec 13, 2019

View reviewed changes

RyanLassigBanks merged commit 896b63e into Cargill:master Dec 13, 2019

RyanLassigBanks deleted the peering-manager-mkIV branch April 21, 2020 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection Manager enhancements #371

Connection Manager enhancements #371

RyanLassigBanks commented Dec 5, 2019 •

edited

vaporos Dec 12, 2019

RyanLassigBanks Dec 12, 2019

ltseeley Dec 13, 2019

ltseeley Dec 13, 2019

ltseeley Dec 13, 2019

Connection Manager enhancements #371

Connection Manager enhancements #371

Conversation

RyanLassigBanks commented Dec 5, 2019 • edited

vaporos Dec 12, 2019

Choose a reason for hiding this comment

RyanLassigBanks Dec 12, 2019

Choose a reason for hiding this comment

ltseeley Dec 13, 2019

Choose a reason for hiding this comment

ltseeley Dec 13, 2019

Choose a reason for hiding this comment

ltseeley Dec 13, 2019

Choose a reason for hiding this comment

RyanLassigBanks commented Dec 5, 2019 •

edited