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

Allow splitting client to support concurrent read/publish #27

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Wazner
Copy link

@Wazner Wazner commented Sep 14, 2021

I'm using this library from a web application used to control IoT devices. I would like to publish a message in response to a web call.
The current API does not allow concurrent use of the Client.

In this PR I implemented a into_split method which, much like the tokio::net::TcpStream::into_split will split the Client into two halves: a reading half and a publishing half.
The publishing half is Sync and clonable, which allows use from multiple threads.
The reading half is not, and remains usable from only a single thread.

The current implementation in this PR is very rough (just copied various methods from the Client) and serves my purpose.
If the maintainer of this repository is interested in this functionality, I will clean it up further.

EDIT:
API design changed a bit since original design described above. See #27 (comment)

Copy link
Contributor

@marcelbuesing marcelbuesing left a comment

Choose a reason for hiding this comment

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

👍 I just added some comments. But I think e.g. the expects seem to be part of the existing code base anyway already.

@fluffysquirrels
Copy link
Owner

I think this is an interesting feature request. Unfortunately I have issues with the way it's currently implemented in your branch. From a brief look it seems quite a lot of code is duplicated between the new ClientReadHalf and ClientPublishHalf implementations and the original Client, and I think this would be quite hard to maintain correctly.

Is there a way for the Client*Half implementations to delegate to an inner Client, or for all of Client*Half and Client to delegate to a new ClientInner that holds most of the logic?

@Wazner
Copy link
Author

Wazner commented Sep 19, 2021

Thank you for the feedback. I'll work on it sometime this week:

  • refactor to remove code duplication. Probably remove send functions from client, and make them functions accept individual parts instead of client struct.
  • add better documentation, with example

@Wazner
Copy link
Author

Wazner commented Sep 21, 2021

I have changed the API to something I am more comfortable with, removed code duplication and added some documentation.

The new API works by allowing the creation of ClientPublisher instances (similar to the PublishHalf in the earlier concept). These instances can be cloned and are Send (so they can be used from different threads).

An example how this works:

use mqtt_async_client::client::Client;
 
let client = ..;
  
let publisher1 = client.publisher();
let publisher2 = client.publisher();  // Or use publisher1.clone()

tokio::spawn(async {
  publisher1.publish(..)
});

tokio::spawn(async {
  publisher2.publish(..)
});

The original Client instance remains usable after creating one or more publishers.

@Wazner Wazner marked this pull request as ready for review September 21, 2021 11:03
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.

3 participants