Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

Pass back heartbeat futures instead of manually spawning #3

Closed
whitfin opened this issue Feb 24, 2019 · 7 comments
Closed

Pass back heartbeat futures instead of manually spawning #3

whitfin opened this issue Feb 24, 2019 · 7 comments

Comments

@whitfin
Copy link

whitfin commented Feb 24, 2019

It would be better to avoid assuming that the caller is using Tokio, by wrapping up the heartbeat future and passing it back to the caller so they can spawn it themselves. The current API is quite clunky with the whole handle concept.

It would be better if (just like the non-TLS version), I received (client, heartbeat) and could do whatever I want with it. This can be accomplished by using join or select wherever you would currently spawn something.

@Keruspe
Copy link
Member

Keruspe commented Feb 24, 2019

It used to be like that, it’s an intended change.
I could add another method that does just that if needed though, but the current one won’t change right now.
My plans would rather be taking an Executor as a parameter and using it instead of hard coding the tokio one

@Keruspe
Copy link
Member

Keruspe commented Feb 24, 2019

A new lapin release should happen in the next few days, and a new release of tokio-rustls too, I could add such a method at this moment I guess, there’s no harm in giving people some choice :)

@whitfin
Copy link
Author

whitfin commented Feb 24, 2019

@Keruspe another method would be very good for the time being. Not only is it more general, but it also aids migration from the non-TLS library (because you just swap out the connection creation, and everything else stays the same). My current non-TLS setup passes the heartbeat back to the main caller, but I can't do that easily anymore when using TLS.

Thank you for replying so fast!

@whitfin
Copy link
Author

whitfin commented Feb 24, 2019

@Keruspe I can attempt to PR this, if you'd like, although I have the feeling you already have something in mind?

@Keruspe
Copy link
Member

Keruspe commented Feb 24, 2019

If you want to take this one, go for it. I was basically going to add another fn to the trait and make the current one use the new one.
I’ll do it at some point this week otherwise

@Keruspe
Copy link
Member

Keruspe commented Mar 1, 2019

Only updated internal as a proof of concept, but would that suit you? 71a4b15

@Keruspe
Copy link
Member

Keruspe commented Mar 1, 2019

(Will update the others and make a release once I've done a new lapin release first)

@Keruspe Keruspe closed this as completed Mar 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants