> On Nov. 8, 2013, 12:39 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/dispatch.hpp, lines 92-93
> > <https://reviews.apache.org/r/15319/diff/1/?file=380494#file380494line92>
> >
> > Why can't we capture this in Promise::associate instead? I think we
> > want these semantics even for code as simple as this:
> >
> > Future<T> future;
> > Promise<T> promise;
> > promise.associate(future);
> > promise.future().discard();
> > CHECK(future.isDiscarded());
> >
> >
> > What about changing Promise::associate thusly:
> >
> > template <typename T>
> > bool Promise<T>::associate(const Future<T>& future)
> > {
> > if (!f.isPending()) {
> > return false;
> > }
> >
> > future
> > .onReady(std::tr1::bind(&Future<T>::set, f,
> > std::tr1::placeholders::_1))
> > .onFailed(std::tr1::bind(&Future<T>::fail, f,
> > std::tr1::placeholders::_1))
> > .onDiscarded(std::tr1::bind(&Future<T>::discard, f));
> >
> > f.onDiscarded(std::tr1::bind(&Future<T>::discard, future)); // You
> > might need a copy of future to get this to compile since discard is not a
> > const function.
> >
> > return true;
> > }
> >
> > Also, maybe we should add a TODO that makes it such that you can't both
> > associate a promise and set a promise ... but we can do that in a later
> > commit.
>
> Jie Yu wrote:
> Ben, I thought about this today. I need to clarify the semantics here:
>
> Case 1:
> Future<T> future;
> Promise<T> promise;
> promise.future().discard();
> promise.associate(future);
>
>
> Case 2:
> Future<T> future;
> Promise<T> promise;
> promise.associate(future);
> promise.future().discard();
>
> In case2, obviously, 'future' should be discarded. However, in case1, do
> you think 'future' should be discarded or not? One may argue that since
> 'promise.future()' is discarded before the association, we should not discard
> 'future' (just like you did in your code). However, if we don't, we may run
> into issues in chaining dispatch future discarding.
>
> For example:
> class Foo : public Process<Foo> {
> Future<bool> func() { return future; }
> Future<bool> future;
> };
>
> Future<bool> future = dispatch(pid, &Foo::func);
> future.discard();
>
> We would expect at this moment, the 'future' field in class Foo should
> have been discarded. However, it's likely that the association has not
> happened yet (i.e., pdispatcher has not been called yet), thus 'future' field
> in class Foo may not be discarded if I follow the semantics in your code.
>
> Any thoughts on this?
>
> Also, can you elaborate why 'you can't both associate a promise and set a
> promise'? Thanks!
>
> Jie Yu wrote:
> The following code works (tested and make check passes), but has a
> different semantics as yours.
>
> template <typename T>
> bool Promise<T>::associate(const Future<T>& future)
> {
> f.onDiscarded(std::tr1::bind(&Future<T>::discard, future));
>
> if (!f.isPending()) {
> return false;
> }
>
> future
> .onReady(std::tr1::bind(&Future<T>::set, f,
> std::tr1::placeholders::_1))
> .onFailed(std::tr1::bind(&Future<T>::fail, f,
> std::tr1::placeholders::_1))
> .onDiscarded(std::tr1::bind(&Future<T>::discard, f));
>
> return true;
> }
Hey Jie!
When I said "you can't both associate a promise and set a promise' I was
referring to the fact that a future should only "transition/be satisfied" once.
When you 'associate' a promise with a future you're effectively
"transitioning/satisfying" the promise's future "in the future" (i.e., whenever
the associated future transitions/is satisfied).
Given the semantics of Promise::associate (as they stand in the code base
today) there is the potential for some weirdness. Consider:
Promise<T> promise1;
Promise<T> promise2;
promise1.associate(promise2.future());
promise1.set(...);
promise2.set(...); // This line has no impact on 'promise1' because it is
already set in the previous line.
Basically, after a promise has been associated we should not let someone set
the promise as well! I like to think of 'promise1' as in another state (i.e.,
ASSOCIATED: still PENDING but not "settable").
Cancellation is trickier. In general, cancellation goes "both ways" (i.e., you
don't need a Promise to discard, you can just use Future::discard). Thus, I
think it's reasonable to move 'f.onDiscarded(...)' to the top of
Promise::associate.
Of course, it's slightly clumsy to remember that cancellation is "both ways"
and that something like the following does not work:
Future<T> future;
Promise<T> promise;
promise.set(...);
promise.associate(future); // Does nothing!
Alternatively, what about making associate create a true alias? I.e., something
like:
template <typename T>
bool Promise<T>::associate(const Future<T>& future)
{
if (!f.isPending() && !future.isPending()) {
return false;
}
f
.onReady(std::tr1::bind(&Future<T>::set, future,
std::tr1::placeholders::_1))
.onFailed(std::tr1::bind(&Future<T>::fail, future,
std::tr1::placeholders::_1))
.onDiscarded(std::tr1::bind(&Future<T>::discard, future));
future
.onReady(std::tr1::bind(&Future<T>::set, f, std::tr1::placeholders::_1))
.onFailed(std::tr1::bind(&Future<T>::fail, f, std::tr1::placeholders::_1))
.onDiscarded(std::tr1::bind(&Future<T>::discard, f));
return true;
}
Of course, there could be a race here, e.g., two threads/processes associating
promises with one another's futures. I believe this is not a problem assuming
that you always associate before you set. Note that I'm deliberately ignoring
the case where there are two threads/processes simultaneously calling associate
on the _same_ promise, but we could protect against that too using the future's
lock. Thoughts?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15319/#review28471
-----------------------------------------------------------
On Nov. 7, 2013, 7:41 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15319/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2013, 7:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Say you have a process defined as follows:
>
> class FooProcess : public Process<FooProcess>
> {
> public:
> Future<bool> func() { return future; }
> private:
> Future<bool> future;
> };
>
> Then you call dispatch that returns a future:
>
> Future<bool> f = dispatch(process, &FooProcess::func);
>
> If the user discards the future 'f', we expect the 'future' field in
> FooProcess also being discarded. However, this is not the case currently.
>
> This patch fixed this issue.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/dispatch.hpp b337a87
> 3rdparty/libprocess/src/tests/process_tests.cpp 7848599
>
> Diff: https://reviews.apache.org/r/15319/diff/
>
>
> Testing
> -------
>
> make check
>
> Also:
> 3rdparty/libprocess/tests --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>
