Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(examples): Fix tower examples #547

Closed
wants to merge 2 commits into from
Closed

fix(examples): Fix tower examples #547

wants to merge 2 commits into from

Conversation

smhmayboudi
Copy link
Contributor

based on [tower issue 547] (tower-rs/tower#547), "The original service is ready, but the clone is not necessarily ready."

Issue Number: #545

based on [tower issue 547] (tower-rs/tower#547), "The original service is ready, but the clone is not necessarily ready."

Issue Number: #545
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not totally sure how this fixes that bug?, because once you produce the future you can call call at any time from that cloned service.

@@ -57,13 +58,9 @@ mod service {
}

fn call(&mut self, req: Request<BoxBody>) -> Self::Future {
let mut channel = self.inner.clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually want to keep it this way because it shows how you can embed async work around the future of the channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another solution which we can apply.

let clone = self.inner.clone();
let mut inner = std::mem::replace(&mut self.inner, clone);
Box::pin(async move {
    // Do extra async work here...
    channel.call(req).await.map_err(Into::into)
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting.....maybe the mem replace works? @olix0r what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is fine?

@smhmayboudi
Copy link
Contributor Author

smhmayboudi commented Jan 27, 2021

The solution is working in my project and your example. The clone is acting properly on the server-side, but not on client-side. It seems the clone needs more time to be ready or somehow have to call pull_ready again.

@moh-abk
Copy link

moh-abk commented Feb 4, 2021

Any update on this @smhmayboudi ?

@smhmayboudi
Copy link
Contributor Author

@LucioFranco Did you reach any conclusion?

@LucioFranco
Copy link
Member

@davidpdrsn since you've been working a lot on tower recently any thoughts on this?

Comment on lines +75 to +77
// Do async work here....

Box::pin(async move {
// Do async work here....

svc.call(req).await
})
Box::pin(self.inner.call(req).err_into::<Self::Error>())
Copy link
Member

@davidpdrsn davidpdrsn Feb 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Do async work here....
Box::pin(async move {
// Do async work here....
svc.call(req).await
})
Box::pin(self.inner.call(req).err_into::<Self::Error>())
let clone = self.inner.clone();
let mut inner = std::mem::replace(&mut self.inner, clone);
Box::pin(async move {
// Do extra async work here...
inner.call(req).await.map_err::<Self::Error>()
})

Comment on lines +61 to +63
// Do extra async work here...

Box::pin(async move {
// Do extra async work here...

channel.call(req).await.map_err(Into::into)
})
Box::pin(self.inner.call(req).err_into::<Self::Error>())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Do extra async work here...
Box::pin(async move {
// Do extra async work here...
channel.call(req).await.map_err(Into::into)
})
Box::pin(self.inner.call(req).err_into::<Self::Error>())
let clone = self.inner.clone();
let mut inner = std::mem::replace(&mut self.inner, clone);
Box::pin(async move {
// Do extra async work here...
inner.call(req).await.map_err::<Self::Error>()
})

This was we still make it clear to users where to add additional async code.

@davidpdrsn
Copy link
Member

I agree with @olix0r. This fix is correct. However as mentioned I think we should use an explicit async block to make it clear to users who to add additional async. I submitted two suggestions.

@davidpdrsn
Copy link
Member

Superseded by #624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants