tower icon indicating copy to clipboard operation
tower copied to clipboard

Question: Buffer and Shared. How to detect Poll::Ready(Err(_))?

Open yokomizor opened this issue 3 years ago • 3 comments

Hi there,

I am trying to combine Buffer and Shared as described in the Shared service doc: https://github.com/tower-rs/tower/blob/7f004da56fa3b18d60f1ee7abe6a209ed6c48488/tower/src/make/make_service/shared.rs#L59-L65

In this example, if svc.poll_ready returns Poll::Ready(Err(_)), then buffered.poll_ready will propagate it and stop calling the inner service, since it is no good anymore.

make however, will keep returning Poll::Ready(Ok(())) and cloning the bad buffered service. https://github.com/tower-rs/tower/blob/7f004da56fa3b18d60f1ee7abe6a209ed6c48488/tower/src/make/make_service/shared.rs#L91-L97

I could not find a way to make the shared service detect this scenario, so I ended up implementing my own shared service with some basic recover logic.

In this scenario, I understand that both svc and buffered should be dropped. The layer above them should be notified somehow so it could try to recover or just propagate the error all the way to the top.

Did I get it right? If yes, then I would say Shared and Buffered don't play well together, and I would also suggest changing the example in the docs.

cheer!

yokomizor avatar Jan 08 '22 03:01 yokomizor

I have just realized that Reconnect exists. I am considering a setup like Shared<Buffer<Reconnect<MakeService, Target>, Request>>.

It would not cover the case Err(_) = MakeService::poll_ready(...) though.

p.s. I am just playing around with tower, not trying to achieve anything specific, so I am not sure if this would be a problem in the real world.

yokomizor avatar Jan 08 '22 07:01 yokomizor

Hm it feels like a bug to me that Shared doesn’t forward poll_ready to the inner service 🤔

although it would require adding a S: Service bound to fix, which I guess is a breaking change 😞

davidpdrsn avatar Jan 08 '22 08:01 davidpdrsn

also type Error = S::Service::Error...

yokomizor avatar Jan 08 '22 09:01 yokomizor