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

Use a shared_future implementation with thousandeyes-futures to support own specialized worker_thread implementation #13

Open
DarthB opened this issue Oct 30, 2020 · 8 comments

Comments

@DarthB
Copy link

DarthB commented Oct 30, 2020

Dear Giannis Georgalis,

thank you for this nice library. I'm thinking about extending it to be used with the shared_future object. But wanted to get your opinion on this first.

First, I wonder you see major drawbacks or obstacles when using shared_future instead of future in the thousandeyes-futures implementation.

I'm working with a custom worker_thread implementation that encapsulates the communication with a third-party software for simulation. The worker_thread allows to enqueue custom tasks that have access to the third-party software over a COM interface. The shared_future are used at two situations. First, they are used to keep track of finished tasks in the worker_thread, especially for error cases that are linked to the third-party software. Second, the caller may store the shared_future to wait for the worker to finish a list of tasks.

Now we want to be able to use 'then' and 'all' in combination with this custom worker_thread implementation.

If you think this is a good solution for our use-case I would fork thousandeyes-futures and integrate these changes.

Best regards
Tim Janus

@DarthB DarthB changed the title Use a shared_future implementation with own specialized worker_thread implementation with shared_future Use a shared_future implementation with thousandeyes-futures to support own specialized worker_thread implementation Oct 30, 2020
@ggeorgalis
Copy link
Contributor

Hello @DarthB,

(I'm assuming you're referring to std::shared_future objects and not some other custom future implementation).

It sounds like the shared_future is a good solution for your use-case and much more versatile than using condition_variables.

Also, extending the library to work with std::shared_future objects would be great indeed. I did want to add support for that eventually, so if you plan to work on that I can fully support and help you.

Note that I don't anticipate any drawbacks and/or obstacles towards that - it should be straightforward. The first thing I'd do would be to define the future type as a template parameter and thus replace all arguments of type std::future<TIn> with FutureType<TIn> where FutureType could be either std::future or std::shared_future.

Then, the return type of then() and all() would depend on the input FutureType - I.e. it would be std::future if the input is std::future and std::shared_future otherwise.

I think this design is simpler and more clear to understand and extend. I don't think it's worth implementing support for mixing std::future and std::shared_future objects in then() and all() calls.

Let me know what you think about the above and if you have any questions in order to get started.

@DarthB
Copy link
Author

DarthB commented Nov 4, 2020

Hello @ggeorgalis,

thank you for the informative reply.

Yes, I meant the std::shared_future and I also think an additional template parameter is the best solution.
In our use case we do not need to support mixing std::future and std::shared_future in then() and all() and I would therefore prefer to not overcomplicate the then() and all() template functions.

I'll start to work on this on the weekend or Wednesday next week and will keep you up-to-date.

@DarthB
Copy link
Author

DarthB commented Nov 17, 2020

Hello @ggeorgalis

a small update from my side. I needed some time to figure out how SFINAE excactly work. But now I think I have a good basis. Nevertheless, I ran into a problem that I cannot solve at the moment>

The shared_future type has an implicit constructor for future objects. This leads to ambiguous overloads with my current SFINAE expressions. Maybe there is another approach than the one I'm following at the moment or a better way to determine the exact type in a signature of a Callable.

Everything I found so far just tests if a callable is compileable when invoked with a specific set of arguments and this is true due to the implicit conversion.

You can find my changes in my fork in the branch:
feature-generic-future-type

Best regards
Tim

@ggeorgalis
Copy link
Contributor

Thanks @DarthB. I'll try to look into your branch carefully this week, however, in the meantime a quick note that the future type should be a template template parameter - e.g.template<typename> typename FutureType.

Then classes with this definition:

template<class TIn, class TOut, class TFunc>
class Foo;

Will now be defined as follows:

template<template<typename> typename TFuture, class TIn, class TOut, class TFunc>
class Foo;

Then mentions of e.g., std::future<TIn> will essentially become TFuture<TIn>.

Implicit constructors are indeed bad. Check in general if I've missed marking any single-argument ctors as explicit in the project.

@DarthB
Copy link
Author

DarthB commented Nov 18, 2020

Hey, this was a good hint. This syntax makes it a lot easier. then is working as intended now.

@ggeorgalis
Copy link
Contributor

Hello Tim,

Sorry for the late response. I was swamped during the last two weeks and I forgot to answer here. I'm glad this resolved the ambiguity.

Then, whenever you're ready, you can put up a PR and I'll start reviewing it asap. In the meantime let me know if you have any other questions/issues.

@ggeorgalis
Copy link
Contributor

Hello @DarthB are you ready to create a PR with your code? Do you have any questions/concerns before creating the PR?

@DarthB
Copy link
Author

DarthB commented Jan 14, 2021

Hello @ggeorgalis, was quite busy with other works, I will check the code and comments and create the PR during the next week.
Best regards
Tim

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

No branches or pull requests

2 participants