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

Usability: Decouple API from thread model #100

Closed
48656c6c6f opened this issue Nov 1, 2021 · 8 comments
Closed

Usability: Decouple API from thread model #100

48656c6c6f opened this issue Nov 1, 2021 · 8 comments
Assignees
Labels

Comments

@48656c6c6f
Copy link

48656c6c6f commented Nov 1, 2021

The current architecture forces a specific thread model that does not suite all situations. I would like to see threads moved out of the library, allowing the application to decide on the thread model to utilize.

An alternative could be to support two model, one with threads and one without. But, I think offering the async thread model as a separate library that builds on top of this one would be best. I know there is already SyncClient, but its composed of the async client, meaning the thread pool is still created and used.

@sighingnow
Copy link
Member

For some ops, e.g., set, get, it is quite doable to have a implementation without creating new thread as the SyncClient, for some ops, e.g., lock, we will need to maintain a lease and keepalive it periodly in the background to implements the correct semantic.

I have proposed another appoarch in #12, where a seperate thread pool will be associated to a client, and the thread pool will be destroyed once the client got destructed. How do you think about such solution ?

BTW may I know the background about why you need to avoiding creating thread?

@sighingnow sighingnow reopened this Nov 5, 2021
@48656c6c6f
Copy link
Author

48656c6c6f commented Nov 5, 2021

My use case is a distributed system. Depending on the platform, there may be limited threads while most are needed for other purposes. Beyond that, forcing a particular threading model reduces usability across all use cases. The API could be refactored to expect functions to be called within threads created by the application.

True downsides of the present approach include assumptions around signal handling, size of thread pool, and number of dependencies. Are the threads created using the parents signal mask or with all blocked? How might that affect an application? Size of thread pool relative to the CPU and competing priorities?

Do one thing really well and provide an interface for others to do an additional thing on top. In other words, there’s no reason the thread model need be tied to the implementation of the client.

@48656c6c6f
Copy link
Author

Re #12, a compromise would be to allow the consumer of the API to provide an implementation of a thread pool interface, thereby allowing the application to control thread creation. I still feel removal of threading from the library entirely is the better approach.

@sighingnow
Copy link
Member

Agree with the points above.

I will try to revise the implementation the SyncClient, and makes the Client a wrapper around the SyncClient. Some operations, e.g., lock, watch, keepalive, will be hard to use under the sync client, as it requires the user to maintain more contexts and the library will rely on those contexts. But with Client (the async interface), those ops will be more friendly.

I have try to split a build target etcd-cpp-apiv3-core to contains the sync part to avoid requires the boost asio and initializing the internal thread pool inside cpprestsdk, even it doesn't require that.

Thanks for the insights, your driven cases has convinced me.

@sighingnow sighingnow self-assigned this Nov 5, 2021
@sighingnow
Copy link
Member

BTW supporting the users to "provide a thread pool interface" won't happen soon, IMO, as we requires cpprestsdk to work, it will initialize its own boost thread pool, and I haven't find a good alternative for the future/promise feature in the cpprestsdk.

@48656c6c6f
Copy link
Author

What I'm saying is that cpprestsdk should be removed from the library and left to the consumer to decide whether to use it. That said, there are sufficient differences in approach that I will need to move forward with a separate implementation.

@sighingnow
Copy link
Member

What I'm saying is that cpprestsdk should be removed from the library and left to the consumer to decide whether to use it. That said, there are sufficient differences in approach that I will need to move forward with a separate implementation.

Yes. In my previous comment, where the cpprestsdk will be removed from the etcd-cpp-apiv3-core, where I think only grpc and protobuf will be included.

And an extra wrapper etcd-cpp-apiv3 will includes the async client and depends on cpprestsdk.

Here etcd-cpp-apiv3-core and etcd-cpp-apiv3 are referred to CMake build targets.

@sighingnow
Copy link
Member

What I'm saying is that cpprestsdk should be removed from the library and left to the consumer to decide whether to use it. That said, there are sufficient differences in approach that I will need to move forward with a separate implementation.

Yes. In my previous comment, where the cpprestsdk will be removed from the etcd-cpp-apiv3-core, where I think only grpc and protobuf will be included.

And an extra wrapper etcd-cpp-apiv3 will includes the async client and depends on cpprestsdk.

Here etcd-cpp-apiv3-core and etcd-cpp-apiv3 are referred to CMake build targets.

The decouple of API and thread model is implemented and documented in #130

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

No branches or pull requests

2 participants