Page MenuHomePhabricator

Resolved future can not be converted to Eina_Value
Closed, ResolvedPublic


Little problem with Eina_Future. It is not possible to chain eina_future_resolved with eina_future_as_value, because as soon as a future is resolved it call _eina_promise_deliver which call _eina_promise_unlink. This remove the pointer to the promise which eina_future_as_value is looking for with _scheduler_get to build a promise.

I am guessing we should delay the unlink to later (Maybe when we actually do deliver the future), but I am not to sure of the other assertion in the code.

This is not a problem that can be worked around as you can not control the future you get from a function call and if it is resolved, any chain you build out of it, will trigger this bug.

cedric created this task.Mar 2 2018, 4:53 PM
cedric triaged this task as High priority.

Hi Cedric,

I think this needs to be addressed at

if (value.type == &EINA_VALUE_TYPE_PROMISE)
  _eina_promise_value_steal_and_link(p->scheduler, value, f); // <-- here should handle it

As you can see in the eina_future_as_value() uses EINA_VALUE_TYPE_PROMISE underneath.

I can't look/debug right now, but the following code is likely where you need to keep the chain

static void
_eina_promise_value_steal_and_link(Eina_Future_Scheduler *scheduler,
                                   Eina_Value value,
                                   Eina_Future *f)
   Eina_Promise *p = _eina_value_promise_steal(&value);
   DBG("Promise %p stolen from value", p);
   // In the case of continue promise, define a scheduler when steal&link
   if (!p->scheduler) p->scheduler = scheduler;
   if (f) _eina_promise_link(p, f);
   else _eina_promise_unlink(p);

The problem is that the test is coming to late. This line is called first with a call to eina_promise_resolve when value.type != EINA_VALUE_TYPE_PROMISE. This mean that we do not call the steal and link. And when the eina_future_as_value is coming, it is not reaching that point.

Do you remember why we do unlink so early on and not once we are going to call the callback ?

could you provide a .c and a call log so I can try to see what's happening? To me I don't see how "it's too late", _eina_promise_unlink(p) just isolates it, but you get the scheduler as the first parameter of _value_steal_and_link(), alongside with the value (which contains the other/child promise) and the future... so all the information should be there.

I don't recall the details, but the rule of thumb was to unlink everything as soon as possible to avoid cycles, such as from the callback one cancel -- it shouldn't work, but this would prevent from things like that propagating... something like "walking" counters in other EFL.

To me it still looks like postponing the unlink is solving the problem at incorrect place.

cedric added a comment.Mar 5 2018, 1:29 PM
#include <Efl_Core.h>

static Eina_Value
_job(void *data, const Eina_Value v, const Eina_Future *dead_future EINA_UNUSED)
   Efl_Loop *loop = data;
   Eina_Future_Scheduler *sched = efl_loop_future_scheduler_get(loop);

   return eina_future_as_value(eina_future_resolved(sched, EINA_VALUE_EMPTY));

static Eina_Value
_work(void *data, const Eina_Value v, const Eina_Future *dead_future EINA_UNUSED)
   Efl_Loop *loop = data;

   efl_loop_quit(loop, eina_value_char_init(0));
   return v;

efl_main(void *data EINA_UNUSED,
         const Efl_Event *ev)
   Eina_Future *f;

   f = efl_loop_job(ev->object);
   f = eina_future_then(f, _job, ev->object);
   eina_future_then(f, _work, ev->object);


You can compile it with :

gcc `pkg-config --cflags --libs efl-core` test.c -o test
cedric added a project: efl.Mar 8 2018, 9:46 AM

Basically the future returned by eina_future_resolved has already lost his promise reference, so eina_future_as_value can not do anything about it. As said, it is to late.

Hi Cedric,

thanks for your finding... that still doesn't look like the problem: we're returning it as a future return, then the caller of _job could do the linkage... maybe that's the missing bit?

From a quick look, it would be _eina_future_dispatch() the one that I'm talking about: Since the returned Eina_Value is a promise:

if (next_value.type == &EINA_VALUE_TYPE_PROMISE)
     if (EINA_UNLIKELY(eina_log_domain_level_check(_promise_log_dom, EINA_LOG_LEVEL_DBG)))
          Eina_Promise *p = NULL;

          eina_value_pget(&next_value, &p);
          DBG("Future %p will wait for a new promise %p", f, p);
     _eina_promise_value_steal_and_link(scheduler, next_value, f);

which is the same function I was talking before: _eina_promise_value_steal_and_link. The next_value's p->scheduler would be set to the current scheduler, that is, the one that called _job in your test. I'd dig in this part of the code, likely just setting p->scheduler = scheduler is not enough?

NOTE: I'm assuming the value is being created, but maybe eina_future_as_value() is returning EINA_VALUE_EMPTY... let me know if that's the case...
cedric added a comment.Mar 9 2018, 9:26 AM

I think the problem come from which unlink the promise from the future before returning it during the call of eina_promise_resolve inside eina_future_resolved. So when eina_future_as_value, it has no way to find a scheduler as the only reference was removed on the future before it could get it. At least that's how I understand the flow of code.

I still don't understand why we need to unlink straight at the point of the resolve and we can not wait for later.

Cedric, could you confirm that eina_future_as_value() IS returning something valid (ie: non-EINA_VALUE_EMPTY?

No, it does abort in eina_future_as_value as it can't get a scheduler from the future it is given.

ohh... then it makes more sense.

Try the following:

static Eina_Future_Scheduler *
_scheduler_get(Eina_Future *f)
   for (; f->prev != NULL; f = f->prev)
        if (f->promise) return f->promise->scheduler;
        else if (f->scheduled_entry) return f->scheduled_entry->scheduler;
   assert(EINA_FALSE && "no scheduler for future!");
   return NULL;

This is correct, because it was unlinked but it was scheduled... and the scheduled entry has the pointer to the scheduler.

I hope it was the missing part ;-)