Add SharedFuture

[techleeksnap]

Add SharedFuture, a C++ wrapper around djinni::Future to allow multiple consumers (i.e. like std::shared_future)

[techleeksnap]

@LiFengSC These are translated from the existing cpp tests

[jb-gcx]

Disclaimer: I'm not a maintainer, so any feedback from me is not binding. I just reviewed because I've done some work on the futures recently.

In my opinion this is a useful addition, thank you for taking the time 🙏

[techleeksnap]

good call

[li-feng-sc]

Yes. The future class was mainly created for bridging with other languages so this is not a super important limitation as we can't have a reference to a C++ object in other languages anyway.

[techleeksnap]

good call, done.

[li-feng-sc]

yes this is a difference from Future, where you transform a ready future instead of the result. This was intentional so that the transform function gets to handle exceptions.

[techleeksnap]

Good point regarding exception handling. Changed to handle similarly to Future.

Question: is it valid to have this return a Future? Then it costs less resources by default and the user can decide whether to make it into a SharedFuture or not.

Since this class really is just for convenience, I would opt to keep this chaining to SharedFutures. I think it makes more sense semantically and is cleaner to produce more SharedFutures. Otherwise, this could have just been a simple helper method cloneFuture(Future<T>& f) -> Future<T> instead of a full blown class.

Finally, regarding remove_cvref_t, that's what I used to have, but that is only available in C++20, and we are still configured as c++ 17 for now.

[techleeksnap]

I think the advantage of returning a regular Future is that it a) uses less resources and b) can deal with move-only returnvalues. For example: a transform functor returning a unique_ptr will be extremely awkward if then returns a SharedFuture.

I agree about resources.
But it turns out djnni::Future actually doesn't work with move-only types, anyway. At least that was the case last time I tried; is it different now?

You can always make a Future into a SharedFuture, but never the other way around.

Well you can just call toFuture() on the result. it's just a tradeoff of resource vs. convenience.

I'd be very interested in an example or explanation of when it's inconvenient to return Future instead of SharedFuture.

Sure. I added this class because it proves to be challenging to implement a djinni interface that is based on Futures. Specifically, you have a bunch of intermediate results that are Futures, and you need to apply some processing on them when the caller invokes methods returning other Futures. I practice, I always have a bunch of Futures that internally depend on other Futures, in the form of a DAG. This means I needed additional resolved result storage solution for each Future. SharedFuture solved this issue, and I needed all these intermediate results to be SharedFutures, as well. I only ever produce regular Futures again when I finally export them out of the djinni boundary to another language.

[techleeksnap]

So the code is usually something like

SharedFuture<Something> inputA;
SharedFuture<SomethingElse> inputB;

void someInit() {
  SharedFuture<C> derived = something(inputA);
  _someMember = someLogic(inputA, derived);
  _otherMember = otherLogic(derived);
}

Future<Foo> someAccessor() {
  return someLogic(_someMember);
}

Future<Bar> otherAccessor() {
  return someLogic(_someMember, _otherMember);
}

So it's just cleaner if then produces SharedFuture by default, so I don't have to wrap each result into SharedFuture explicitly 90% of the time.

In any case, I updated the code to include both flavors.

[li-feng-sc]

I don't have a strong opinion on this. It kind of makes sense that once someone starts to use SharedFuture they'd want to keep using it.

[jb-gcx]

Thanks for updating the code. I've got no more complaints, looks great 🎉

[techleeksnap]

Yeah that's fair. Originally, the lambdas took resolved values instead of the futures, so the overload was kind of needed for SharedFuture. Removed for now.

[li-feng-sc]

yes this is a difference from Future, where you transform a ready future instead of the result. This was intentional so that the transform function gets to handle exceptions.

[li-feng-sc]

Yes. The future class was mainly created for bridging with other languages so this is not a super important limitation as we can't have a reference to a C++ object in other languages anyway.

[jb-gcx]

FYI: I can't resolve my own review comments due to GH weirdness. I think only a maintainer and the creator of the PR can resolve them which is strange 🙃

I've already deleted one comment, but I decided I don't like just deleting addressed comments. Instead I'll ping @techleeksnap to resolve the ones that I consider done. Sorry for the inconvenience 🙏

Edit: Nevermind, I just discovered that I can use Hide to mark comments as resolved. Which is weird, but okay. Sorry for the spam people, Github and I are not getting along today.

[techleeksnap]

good catch

[techleeksnap]

@LiFengSC Can you approve the Build and Test workflow, again?

1 workflow awaiting approval
This workflow requires approval from a maintainer. [Learn more about approving workflows.](https://docs.github.com/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks)
1 expected check