From e2474c3d879f4d1bfc8a2ab2760a95b68784e3a3 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 15 Apr 2024 10:23:50 +0100 Subject: [PATCH 1/2] DOCS: Comments about consensus concurrency control --- README.md | 7 ++++++- examples/kvstore_34/main.rs | 3 +++ examples/kvstore_37/main.rs | 3 +++ examples/kvstore_38/main.rs | 3 +++ src/buffer4/service.rs | 6 ++++++ 5 files changed, 21 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 5a8299c..7742f4f 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,11 @@ processing of some requests by immediately returning a future that will be executed on the caller's task. Although all requests are still received by the application task, not all request processing needs to happen on the application task. +At this level the developer must pay closer attention to utilising Tower +layers to control the concurrency of the individual services mentioned above. +In particular the `Consensus` service should be wrapped with +`ServiceBuilder::concurrency_limit` to avoid a potential reordering of +consensus message effects caused by concurrent execution. 3. At the highest level of complexity, application developers can implement multiple distinct `Service`s and manually control synchronization of shared @@ -65,4 +70,4 @@ services, etc. [ABCI]: https://docs.tendermint.com/master/spec/abci/ [Tower]: https://docs.rs/tower -[svc]: https://docs.rs/tower/0.4.6/tower/trait.Service.html \ No newline at end of file +[svc]: https://docs.rs/tower/0.4.6/tower/trait.Service.html diff --git a/examples/kvstore_34/main.rs b/examples/kvstore_34/main.rs index 6c3fac6..35d0513 100644 --- a/examples/kvstore_34/main.rs +++ b/examples/kvstore_34/main.rs @@ -158,6 +158,9 @@ async fn main() { // Hand those components to the ABCI server, but customize request behavior // for each category -- for instance, apply load-shedding only to mempool // and info requests, but not to consensus requests. + // Note that this example use synchronous execution in `Service::call`, wrapping the end result in a ready future. + // If `Service::call` did the actual request handling inside the `async` block as well, then the `consensus` service + // below should be wrapped with a `ServiceBuilder::concurrency_limit` to avoid any unintended reordering of message effects. let server_builder = Server::builder() .consensus(consensus) .snapshot(snapshot) diff --git a/examples/kvstore_37/main.rs b/examples/kvstore_37/main.rs index 3ad5fbb..a12bd39 100644 --- a/examples/kvstore_37/main.rs +++ b/examples/kvstore_37/main.rs @@ -165,6 +165,9 @@ async fn main() { // Hand those components to the ABCI server, but customize request behavior // for each category -- for instance, apply load-shedding only to mempool // and info requests, but not to consensus requests. + // Note that this example use synchronous execution in `Service::call`, wrapping the end result in a ready future. + // If `Service::call` did the actual request handling inside the `async` block as well, then the `consensus` service + // below should be wrapped with a `ServiceBuilder::concurrency_limit` to avoid any unintended reordering of message effects. let server_builder = Server::builder() .consensus(consensus) .snapshot(snapshot) diff --git a/examples/kvstore_38/main.rs b/examples/kvstore_38/main.rs index 2767d90..d1ab435 100644 --- a/examples/kvstore_38/main.rs +++ b/examples/kvstore_38/main.rs @@ -207,6 +207,9 @@ async fn main() { // Hand those components to the ABCI server, but customize request behavior // for each category -- for instance, apply load-shedding only to mempool // and info requests, but not to consensus requests. + // Note that this example use synchronous execution in `Service::call`, wrapping the end result in a ready future. + // If `Service::call` did the actual request handling inside the `async` block as well, then the `consensus` service + // below should be wrapped with a `ServiceBuilder::concurrency_limit` to avoid any unintended reordering of message effects. let server_builder = Server::builder() .consensus(consensus) .snapshot(snapshot) diff --git a/src/buffer4/service.rs b/src/buffer4/service.rs index f38b150..5f87bd2 100644 --- a/src/buffer4/service.rs +++ b/src/buffer4/service.rs @@ -62,6 +62,12 @@ where /// [`poll_ready`] but will not issue a [`call`], which prevents other senders from issuing new /// requests. /// + /// # A note on the scope of `bound` + /// + /// Note that `bound` will only limit the rate of the _submission_ of [Message]s to the [Worker], + /// not their _execution_. If the execution itself is asynchronous, concurrency should be further + /// controlled by applying an appropriate [tower::Layer] on the returned service component. + /// /// [`Poll::Ready`]: std::task::Poll::Ready /// [`call`]: crate::Service::call /// [`poll_ready`]: crate::Service::poll_ready From c43b1d4c479908fc9589f825e7e4626fc5e12473 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 15 Apr 2024 13:02:00 +0100 Subject: [PATCH 2/2] DOCS: Hint about deadlock if concurrency_limit is applied without buffer --- README.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 7742f4f..5ca4c27 100644 --- a/README.md +++ b/README.md @@ -54,8 +54,10 @@ application task. At this level the developer must pay closer attention to utilising Tower layers to control the concurrency of the individual services mentioned above. In particular the `Consensus` service should be wrapped with -`ServiceBuilder::concurrency_limit` to avoid a potential reordering of -consensus message effects caused by concurrent execution. +`ServiceBuilder::concurrency_limit` of 1 to avoid a potential reordering of +consensus message effects caused by concurrent execution, as well as +`ServiceBuilder::buffer` to avoid any deadlocks in message handling in `Connection` +due to the limited concurrency. 3. At the highest level of complexity, application developers can implement multiple distinct `Service`s and manually control synchronization of shared