[transport] Add binding port to SpawnedServer #861

patrickkuo · 2022-03-16T12:54:07Z

This is an improvement to SpawnedServer to expose actual binding tcp port, this allow us to start the server with port 0 and retrieve the binding port for later use.

add binding port to SpawnedServer
cli test use SpawnedServer's binding port instead of hardcoding ports
remove wait for "Listening to TCP traffic on 127.0.0.1" log output, start_test_network now block until authorities are up

huitseeker

Two main things:

I think we should expose the whole SocketAddr, we can write the get_port in this PR and we'll deal with e.g. get_address later.
We can probably rename things to be closer than what they are:
- create_network is actually a start_network,
- start is actually a monitor / wait_for_completion / run_to_end.

network_utils/src/transport.rs

huitseeker · 2022-03-17T19:25:27Z

network_utils/src/transport.rs

-        tokio::spawn(run_tcp_server(listener, state, receiver, buffer_size))
-    };
-    Ok(SpawnedServer { complete, handle })
+    // see https://fly.io/blog/the-tokio-1-x-upgrade/#tcplistener-from_std-needs-to-be-set-to-nonblocking


The comment is too detached from the line it's commenting on.

huitseeker · 2022-03-17T19:27:24Z

network_utils/src/transport.rs

@@ -86,22 +91,23 @@ where
    S: MessageHandler<TcpDataStream> + Send + Sync + 'static,
 {
    let (complete, receiver) = futures::channel::oneshot::channel();


This is a cancellation channel, to make this idiiomatic with the rest of the codebase, we could rename (here and in the struct definition), to:

let (tx_cancellation, rx_cancellation) = futures::channel::oneshot::channel();

huitseeker · 2022-03-17T19:28:46Z

network_utils/src/transport.rs

+    let local_addr = std_listener.local_addr()?;
+    let host = local_addr.ip();
+    let port = local_addr.port();
+    info!("Listening to TCP traffic on {}:{}", host, port);


Nit: I think captured identifiers are nice.

huitseeker · 2022-03-17T19:29:17Z

sui/src/sui_commands.rs

-        });
+            info!("Started {} authorities", handles.len());
+            join_all(handles).await;
+            info!("All server stopped.");


Nit: plural

huitseeker · 2022-03-17T19:33:03Z

sui/src/sui_commands.rs

            }
-        });
+            info!("Started {} authorities", handles.len());


By the time you log this, these have long been started. You want to log at the end of create_network instead.

huitseeker · 2022-03-17T19:35:45Z

sui/src/sui_commands.rs

+    let mut spawned_authorities = Vec::new();
    for authority in &config.authorities {
        let server = make_server(authority, &committee, config.buffer_size).await?;
+        spawned_authorities.push(server.spawn().await?);
+    }
+    Ok(SuiNetwork {
+        spawned_authorities,
+    })
+}


This create_networknow requires a comment indicating that if we finish the caller's task (and drop the SuiNetwork) without calling start, all servers are dropped as well.

If we drop the SuiNetwork. but not the caller's task, the servers will continue in detached mode (which is probably why the logging-on-error done currently in start should probably be an obligation on the server).

hmm didn't realise this is behaving this way.... thanks for pointing this out!

In this case I think maybe better moving the create_network into SuiNetwork, so we don't have detached network running unexpectedly.

huitseeker · 2022-03-17T19:39:58Z

sui/src/unit_tests/cli_tests.rs

+    let network = create_network(&network_config).await?;

-    SuiCommand::Genesis {
-        working_dir,
-        config: Some(genesis_conf_path),
+    let network_config = network_config.persisted(&network_path);
+    network_config.save()?;
+    keystore.save(&keystore_path)?;
+
+    let authorities = network_config.get_authority_infos();
+    let authorities = authorities
+        .into_iter()
+        .zip(&network.spawned_authorities)
+        .map(|(mut info, server)| {
+            info.base_port = server.get_port();
+            info
+        })
+        .collect();
+
+    // Create wallet.conf with stated authorities port
+    WalletConfig {
+        accounts,
+        keystore: KeystoreType::File(keystore_path),
+        gateway: GatewayType::Embedded(EmbeddedGatewayConfig {
+            db_folder_path,
+            authorities,
+            ..Default::default()
+        }),
    }
-    .execute()
-    .await?;
+    .persisted(&wallet_path)
+    .save()?;

    // Start network
-    let network = task::spawn(async move {
-        SuiCommand::Start {
-            config: network_conf_path,
-        }
-        .execute()
-        .await
-    });
-    Ok(network)
+    Ok(network.start())


The renames I'm suggesting should make this function a lot more understandable: the network is actually started way before start.

666lcz

Love this! The tests are so much cleaner now.

666lcz · 2022-03-17T19:21:52Z

sui/src/unit_tests/cli_tests.rs

+    let (network_config, accounts, keystore) = genesis(genesis_config).await?;
+    let network = create_network(&network_config).await?;

-    SuiCommand::Genesis {
-        working_dir,
-        config: Some(genesis_conf_path),
+    let network_config = network_config.persisted(&network_path);
+    network_config.save()?;
+    keystore.save(&keystore_path)?;


Will these files ever get deleted?

we use temp_dir in test, so the folder will get deleted automatically after the test

666lcz · 2022-03-17T19:30:34Z

sui/src/sui_commands.rs

+
+impl SuiNetwork {
+    pub fn start(self) -> JoinHandle<Result<(), anyhow::Error>> {
+        task::spawn(async move {


N00b question: why do we spawn these spawned_server(which have been spawned on line 129) again?

This extra spawn is redundant! we can wait for completion in the same thread.

666lcz · 2022-03-17T19:41:12Z

sui/src/unit_tests/cli_tests.rs

+        .into_iter()
+        .zip(&network.spawned_authorities)
+        .map(|(mut info, server)| {
+            info.base_port = server.get_port();


So how does the non-test code path work? Where do we ensure the port number stored in network.conf matches the one in spawned servers?

in non-test path, if the user decided to start the server using port 0, is their responsibility to manually change the wallet.conf to align the port.

the servers will be spawned using ports provided by network.conf, we will not modify the config to align the port.

When the configured port number is 0, the user can get the actual binding port number from the console/log output.

* cli test use SpawnedServer's binding port instead of hardcoding ports * remove wait for "Listening to TCP traffic on 127.0.0.1" log output, `start_test_network` now block until authorities are up

patrickkuo · 2022-03-18T11:29:23Z

I have addressed all comments, more review please :)

huitseeker

Thanks a bunch, with the rename things make much more sense!
So happy we'll now be killing this bug!

patrickkuo force-pushed the pat/config_refactoring branch 2 times, most recently from 625cbe4 to 2c27da9 Compare March 16, 2022 15:09

patrickkuo force-pushed the pat/spawned_server_port branch from 098f813 to bf957fd Compare March 16, 2022 15:10

Base automatically changed from pat/config_refactoring to main March 16, 2022 16:04

patrickkuo force-pushed the pat/spawned_server_port branch from bf957fd to d5dc98d Compare March 16, 2022 16:07

patrickkuo requested review from oxade, 666lcz and huitseeker March 16, 2022 16:08

patrickkuo self-assigned this Mar 16, 2022

patrickkuo marked this pull request as ready for review March 16, 2022 16:08

patrickkuo requested review from gdanezis and sblackshear March 16, 2022 21:32

patrickkuo force-pushed the pat/spawned_server_port branch from d5dc98d to 866e2a6 Compare March 17, 2022 17:08

huitseeker reviewed Mar 17, 2022

View reviewed changes

666lcz reviewed Mar 17, 2022

View reviewed changes

patrickkuo added 3 commits March 18, 2022 00:15

* add binding port to SpawnedServer

af784eb

* cli test use SpawnedServer's binding port instead of hardcoding ports * remove wait for "Listening to TCP traffic on 127.0.0.1" log output, `start_test_network` now block until authorities are up

address PR comments

07bc7ad

fixup after rebase

0cbcc65

patrickkuo force-pushed the pat/spawned_server_port branch from f3da9b6 to 0cbcc65 Compare March 18, 2022 00:18

patrickkuo requested review from huitseeker and 666lcz March 18, 2022 11:29

huitseeker approved these changes Mar 18, 2022

View reviewed changes

patrickkuo merged commit ec394db into main Mar 18, 2022

patrickkuo deleted the pat/spawned_server_port branch March 18, 2022 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[transport] Add binding port to SpawnedServer #861

[transport] Add binding port to SpawnedServer #861

patrickkuo commented Mar 16, 2022

huitseeker left a comment

huitseeker Mar 17, 2022

huitseeker Mar 17, 2022

huitseeker Mar 17, 2022

huitseeker Mar 17, 2022

huitseeker Mar 17, 2022

huitseeker Mar 17, 2022

patrickkuo Mar 17, 2022

huitseeker Mar 17, 2022

666lcz left a comment

666lcz Mar 17, 2022

patrickkuo Mar 17, 2022

666lcz Mar 17, 2022

patrickkuo Mar 18, 2022

666lcz Mar 17, 2022

patrickkuo Mar 18, 2022

patrickkuo Mar 18, 2022

patrickkuo commented Mar 18, 2022

huitseeker left a comment

[transport] Add binding port to SpawnedServer #861

[transport] Add binding port to SpawnedServer #861

Conversation

patrickkuo commented Mar 16, 2022

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickkuo commented Mar 18, 2022

huitseeker left a comment

Choose a reason for hiding this comment