Fix buffered service issue #5

New issue

Closed

opened 2022-05-24 15:56:42 +00:00 by KokaKiwi · 1 comment

KokaKiwi commented

2022-05-24 15:56:42 +00:00

Owner

Using a buffered Service Layer seems to be causing a bug in receiving requests

KokaKiwi self-assigned this 2022-05-24 15:58:58 +00:00

KokaKiwi closed this issue

2022-07-01 09:56:28 +00:00

KokaKiwi reopened this issue

2022-07-01 09:56:42 +00:00

KokaKiwi added this to the Working project 2022-07-01 09:57:44 +00:00

quentin commented

2022-08-03 15:11:34 +00:00

TL;DR: I think you can close this issue

I was not able to test the "buffer" middleware on Aerogramme as introducing the ServiceBuilder was breaking my types in too many places in the code.

However, I did some tests based on your example.rs file, both on an old commit (from May 13th) and a recent one (ce339cf89b) I patched with:

diff --git a/examples/simple.rs b/examples/simple.rs
index 5bdd45c..1c6086c 100644
--- a/examples/simple.rs
+++ b/examples/simple.rs
@@ -4,11 +4,13 @@ use boitalettres::proto::{Request, Response};
 use boitalettres::server::accept::addr::{AddrIncoming, AddrStream};
 use boitalettres::server::Server;

-async fn handle_req(_req: Request) -> Result<Response> {
+async fn handle_req(req: Request) -> Result<Response> {
     use imap_codec::types::response::{Capability, Data as ImapData};

     use boitalettres::proto::res::{body::Data, Status};

+    tracing::info!("Got request {:?}", req);
+
     let capabilities = vec![Capability::Imap4Rev1, Capability::Idle];
     let body: Vec<Data> = vec![
         Status::ok("Yeah")?.into(),
@@ -25,11 +27,12 @@ async fn main() -> Result<()> {
     let incoming = AddrIncoming::new("127.0.0.1:4567")
         .await
         .into_diagnostic()?;
+    tracing::info!("Listen on :4567");

     let make_service = tower::service_fn(|addr: &AddrStream| {
-        tracing::debug!(remote_addr = %addr.remote_addr, local_addr = %addr.local_addr, "accept");
+        tracing::info!(remote_addr = %addr.remote_addr, local_addr = %addr.local_addr, "accept");

-        let service = tower::ServiceBuilder::new().service_fn(handle_req);
+        let service = tower::ServiceBuilder::new().buffer(100).service_fn(handle_req);

         futures::future::ok::<_, std::convert::Infallible>(service)
     });
@@ -46,12 +49,7 @@ fn setup_logging() {
     let registry = tracing_subscriber::registry().with(
         tracing_subscriber::fmt::layer().with_filter(
             tracing_subscriber::filter::Targets::new()
-                .with_default(tracing::Level::DEBUG)
-                .with_target("boitalettres", tracing::Level::TRACE)
-                .with_target("simple", tracing::Level::TRACE)
-                .with_target("tower", tracing::Level::TRACE)
-                .with_target("tokio_tower", tracing::Level::TRACE)
-                .with_target("mio", tracing::Level::TRACE),
+                .with_default(tracing::Level::INFO)
         ),
     );

I was able to reproduce the bug on the old commit (the request was not processed). At the same time, I was not able to reproduce the bug on the recent commit, and I tried multiple buffer size. So, I am strongly convinced that the bug is fixed.

**TL;DR: I think you can close this issue** I was not able to test the "buffer" middleware on Aerogramme as introducing the ServiceBuilder was breaking my types in too many places in the code. However, I did some tests based on your example.rs file, both on an old commit (from May 13th) and a recent one (ce339cf89b5742dbdb8eadac9ec7a8fc5703e15e) I patched with: ```diff diff --git a/examples/simple.rs b/examples/simple.rs index 5bdd45c..1c6086c 100644 --- a/examples/simple.rs +++ b/examples/simple.rs @@ -4,11 +4,13 @@ use boitalettres::proto::{Request, Response}; use boitalettres::server::accept::addr::{AddrIncoming, AddrStream}; use boitalettres::server::Server; -async fn handle_req(_req: Request) -> Result<Response> { +async fn handle_req(req: Request) -> Result<Response> { use imap_codec::types::response::{Capability, Data as ImapData}; use boitalettres::proto::res::{body::Data, Status}; + tracing::info!("Got request {:?}", req); + let capabilities = vec![Capability::Imap4Rev1, Capability::Idle]; let body: Vec<Data> = vec![ Status::ok("Yeah")?.into(), @@ -25,11 +27,12 @@ async fn main() -> Result<()> { let incoming = AddrIncoming::new("127.0.0.1:4567") .await .into_diagnostic()?; + tracing::info!("Listen on :4567"); let make_service = tower::service_fn(|addr: &AddrStream| { - tracing::debug!(remote_addr = %addr.remote_addr, local_addr = %addr.local_addr, "accept"); + tracing::info!(remote_addr = %addr.remote_addr, local_addr = %addr.local_addr, "accept"); - let service = tower::ServiceBuilder::new().service_fn(handle_req); + let service = tower::ServiceBuilder::new().buffer(100).service_fn(handle_req); futures::future::ok::<_, std::convert::Infallible>(service) }); @@ -46,12 +49,7 @@ fn setup_logging() { let registry = tracing_subscriber::registry().with( tracing_subscriber::fmt::layer().with_filter( tracing_subscriber::filter::Targets::new() - .with_default(tracing::Level::DEBUG) - .with_target("boitalettres", tracing::Level::TRACE) - .with_target("simple", tracing::Level::TRACE) - .with_target("tower", tracing::Level::TRACE) - .with_target("tokio_tower", tracing::Level::TRACE) - .with_target("mio", tracing::Level::TRACE), + .with_default(tracing::Level::INFO) ), ); ``` I was able to reproduce the bug on the old commit (the request was not processed). At the same time, I was **not** able to reproduce the bug on the recent commit, and I tried multiple buffer size. So, I am strongly convinced that the bug is fixed.

KokaKiwi closed this issue

2022-08-03 15:38:02 +00:00