> 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;
> }
>
> Benjamin Hindman wrote:
> 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?
Ben, I thought about the true alias implementation today, and I think it is
very complicated and I cannot find an elegant solution to avoid the race
conditions here.
I checked our code base again today. Seems that in our case, what we usually
want is a one way association. A common pattern is like the following:
// 1 - Give a future to client first. Pass the promise to some other callback.
The actual future is not yet available.
Promise<T> promise;
return promise.future();
// 2 - Associate the actual future with the passed promise so that the client
can be notified when the actual future is ready.
Future<T> future = func(...);
promise.associate(future);
(e.g. dispatch, then, registrar)
We usually don't set/fail the promise directly in these cases. In other words,
we don't need a two way alias in these cases. In fact, the client usually
cannot set/fail the promise directly as the promise is usually not visible to
client. A notable exception is discard because Future can be directly
discarded. Therefore, I think it make sense to make an exception for the
discard case here (just like the exception we made by allowing a Future to be
directly discarded without involving a Promise).
Because of that, I would like to add a TODO here and look into them later. For
now, I will do the following. Let me know your thoughts!
template <typename T>
bool Promise<T>::associate(const Future<T>& future)
{
// TODO(jieyu): Making 'f' a true alias of '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;
}
- Jie
-----------------------------------------------------------
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
>
>
