[network] Don't back off forever on the Semaphore #559

huitseeker · 2022-07-22T02:14:57Z

Context

#463 introduced a BoundedExecutor to limit the concurrent tasks created for sending messages.

However, as a protocol requirement, some of those tasks are on an unbounded exponential backoff, by which they are retried indefinitely (until cancelled).

The issue

The {primary, worker} × { broadcast, send } functions enqueue instances of {Primary, Worker}::send_message on the BoundedExecutor, and send_message is an exponential backoff with infinite retries. These tasks will hence hold a semaphore ticket potentially forever.

This change

The present change makes those tasks hold a semaphore permit only when they are actively sending a message, re-queuing on the semaphore only once they are done with their backoff wait (out of the semaphore0. This frees up tickets for other network tasks to help make the network progress.

Future work

The cancellation of our "reliable" network sends is not always as prompt as it could be. Fixing this is the next PR.

… the semaphore Our BoundedExecutor used to queue messages with unbounded exponential backoff on the semaphore. Now the backoff of those retries occurs outside of the semaphore, the attempts just queue when it's their time to try again.

mystenmark · 2022-07-22T04:08:36Z

network/src/primary.rs

+
+        let message_send = move || {
+            let mut client = client.clone();
+            let message = message.clone();


surely this is not deep-copying the entire payload every time... right?

As of today the message type is essentially a Bytes struct which has a very efficient clone operation (its essentially just an atomic increment)

The PR does not change the cloning behavior, but FWIW the client also has a cheap cloning operation.

network/src/bounded_executor.rs

gdanezis · 2022-07-22T16:17:20Z

network/src/worker.rs

-                        response.expect("we retry forever so this shouldn't fail");
-                    }),
-            )
+            .spawn_with_retries(self.retry_config, message_send)


This does create more tasks right?

Overall, yes. But it does not create more concurrent network messages.

network/src/bounded_executor.rs

We operate an executor with a bound on the concurrent number of messages (see MystenLabs#463, MystenLabs#559, MystenLabs#706). We expect the executors to operate for a long time at this limit (e.g. in recovery situation). The spammy logging is not usfeful This removes the logging of the concurrency bound being hit. Fixes MystenLabs#759

We operate an executor with a bound on the concurrent number of messages (see MystenLabs#463, MystenLabs#559, MystenLabs#706). PR MystenLabs#472 added logging for the bound being hit. We expect the executors to operate for a long time at this limit (e.g. in recovery situation). The spammy logging is not usfeful This removes the logging of the concurrency bound being hit. Fixes MystenLabs#759

) We operate an executor with a bound on the concurrent number of messages (see #463, #559, #706). PR #472 added logging for the bound being hit. We expect the executors to operate for a long time at this limit (e.g. in recovery situation). The spammy logging is not usfeful This removes the logging of the concurrency bound being hit. Fixes #759

…ystenLabs#763) We operate an executor with a bound on the concurrent number of messages (see MystenLabs#463, MystenLabs#559, MystenLabs#706). PR MystenLabs#472 added logging for the bound being hit. We expect the executors to operate for a long time at this limit (e.g. in recovery situation). The spammy logging is not usfeful This removes the logging of the concurrency bound being hit. Fixes MystenLabs#759

) We operate an executor with a bound on the concurrent number of messages (see #463, #559, #706). PR #472 added logging for the bound being hit. We expect the executors to operate for a long time at this limit (e.g. in recovery situation). The spammy logging is not usfeful This removes the logging of the concurrency bound being hit. Fixes #759

…ystenLabs/narwhal#763) We operate an executor with a bound on the concurrent number of messages (see MystenLabs/narwhal#463, MystenLabs/narwhal#559, MystenLabs/narwhal#706). PR MystenLabs/narwhal#472 added logging for the bound being hit. We expect the executors to operate for a long time at this limit (e.g. in recovery situation). The spammy logging is not usfeful This removes the logging of the concurrency bound being hit. Fixes MystenLabs/narwhal#759

huitseeker added 3 commits July 21, 2022 20:55

feat: test panics in BoundedExecutor

74f5baf

refactor: make BoundedExecutor use a future map

0ae9569

huitseeker requested review from gdanezis, asonnino and mystenmark July 22, 2022 02:14

huitseeker requested a review from bmwill as a code owner July 22, 2022 02:14

huitseeker changed the title ~~[network] Don't back off on the Semaphore~~ [network] Don't back off forever on the Semaphore Jul 22, 2022

mystenmark reviewed Jul 22, 2022

View reviewed changes

bmwill reviewed Jul 22, 2022

View reviewed changes

network/src/bounded_executor.rs Outdated Show resolved Hide resolved

gdanezis reviewed Jul 22, 2022

View reviewed changes

akichidis reviewed Jul 22, 2022

View reviewed changes

network/src/bounded_executor.rs Show resolved Hide resolved

fix: improve documentation of spawn_with_retries, remove useless async

495cdb1

huitseeker force-pushed the network_refactor branch from 3faaa61 to 495cdb1 Compare July 22, 2022 16:58

huitseeker added 2 commits July 22, 2022 14:23

fix: make spawn_with_retries pub(crate)

4a64496

fix: assign a BoundedExecutor on a per-address basis

87ac5c8

huitseeker force-pushed the network_refactor branch from d221f9f to 5a4611a Compare July 22, 2022 18:46

fix: more scary warnings

95f6892

huitseeker force-pushed the network_refactor branch from 5a4611a to 95f6892 Compare July 22, 2022 18:50

bmwill approved these changes Jul 22, 2022

View reviewed changes

huitseeker merged commit 7e49fa4 into MystenLabs:main Jul 22, 2022

This was referenced Jul 22, 2022

Cleanup Network #566

Closed

[Network] Operate a retrier for multiple tasks on retry. MystenLabs/sui#5312

Open

Reconfig consensus #489

Merged

huitseeker mentioned this pull request Aug 14, 2022

fix: Avoid spammy logging on hitting the network's BoundedExecutor #763

Merged

huitseeker mentioned this pull request Oct 13, 2022

[network] The BoundedExecutor-per-destinary can cause network starvation MystenLabs/sui#5204

Closed

huitseeker deleted the network_refactor branch January 21, 2023 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[network] Don't back off forever on the Semaphore #559

[network] Don't back off forever on the Semaphore #559

huitseeker commented Jul 22, 2022 •

edited

Loading

mystenmark Jul 22, 2022

bmwill Jul 22, 2022

huitseeker Jul 22, 2022

gdanezis Jul 22, 2022

huitseeker Jul 22, 2022

[network] Don't back off forever on the Semaphore #559

[network] Don't back off forever on the Semaphore #559

Conversation

huitseeker commented Jul 22, 2022 • edited Loading

Context

The issue

This change

Future work

mystenmark Jul 22, 2022

Choose a reason for hiding this comment

bmwill Jul 22, 2022

Choose a reason for hiding this comment

huitseeker Jul 22, 2022

Choose a reason for hiding this comment

gdanezis Jul 22, 2022

Choose a reason for hiding this comment

huitseeker Jul 22, 2022

Choose a reason for hiding this comment

huitseeker commented Jul 22, 2022 •

edited

Loading