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

rumqttc | Unbounded channel #658

Open
carlocorradini opened this issue Jul 6, 2023 · 9 comments
Open

rumqttc | Unbounded channel #658

carlocorradini opened this issue Jul 6, 2023 · 9 comments

Comments

@carlocorradini
Copy link
Contributor

At the moment, the only way to construct a client is to provide a fixed client capacity. A client with unlimited channel capacity would be desirable.
Furthermore, the pure implementation is already available because rumqttc is built on flume. The client/event_loop has to be refactored in order to enable support for both bounded and unbounded channels.
Let me know what you think 🤯

@swanandx
Copy link
Member

Hey, can you elaborate more on why client with unlimited channel capacity would be desirable? cuz, the channel getting full means the client isn't able to process the messages right? so having unlimited capacity will eventually just run out of memory.

@carlocorradini
Copy link
Contributor Author

I don't know what the channel capacity is a priori. I prefer the client to run out of memory (very rare) rather than to block :/
Moreover, it's up to the application designer to decide whether to use a bounded or unbounded channel without forcing it to use a bounded channel.
Finally, since we rely on a library to do all the "channel stuff" we don't even have to rewrite it from scratch :)

These are my ideas but I prefer to also listen yours :)

@swanandx
Copy link
Member

I do agree with you. I think only change required will be to make cap as Option<> and then based on it we can choose to use bounded/unbounded channel.

Would need to give it a little thought on is it really required/helpful, and worth to break the API.
Will get back to you on this, thank you so much!

@carlocorradini
Copy link
Contributor Author

Awesome 😎

@swanandx
Copy link
Member

Hey @carlocorradini , lets do it! Would you like to open PR for this?

@carlocorradini
Copy link
Contributor Author

@swanandx Yeah 🥳

Unfortunately currently I'm unable to develop on my computer but I'll try in the next weeks 🥳

@swanandx
Copy link
Member

no worries, thank for your contribution 💯

@carlocorradini
Copy link
Contributor Author

API idea:

Client::new_bounded(mqttoptions, 10);
Client::new_unbounded(mqttoptions);

and deprecate Client::new(...);

I think this new API is more self descriptive.

What do you think?

@swanandx
Copy link
Member

swanandx commented Jul 28, 2023

That's a great suggestion, but for now I think Option<cap> will work out better. In future, we are planning to have builder pattern for it 😃

e.g. suggested by @de-sh

let client = AsyncClient::builder().host("...").port(...).capacity(...).build();

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 a pull request may close this issue.

2 participants