On Thu, 27 Dec 2018 13:26:19 -0500 Mike Blumenkrantz
<michael.blumenkra...@gmail.com> said:
> Hi,
>
> Thanks for taking the time to look into this issue. There's been an amount
> of discussion and back-and-forth patching related to it for some time now,
> and it's great to see someone taking some initiative here.
>
> Looking over this patch, I'm unsure whether the method used for resolving
> the issue is indeed the optimal one. Given the already-existing discussion
indeed my log comments question the original commit idea - but i thought this
was a bit better than reverting since i had found a non-revert fix. my take is
that "wrapping" eina promise like this is kind of problematic as it means we
now have some "forbidden" calls that are needed to do the wrapping properly
(the ones i added).
> going on related to this topic, I think this is a patch which would have
> benefited from review prior to being landed. In particular, I'd have
> suggested that unit tests be added for this case--we have somewhat
it's still up in the air as to if this is the right way in the end - but i'm
not sure how that helps as these are "public api's" only in as much as we need
them internally but due to having multiple libraires ... we need to expose
them. the documentation reflects that.
there comes a point where you have to weigh up time vs effort. in this case
the time spent adding tests for an internal use case is not worth it IMHO. time
is a limited resource. i don't see how testing these api's in unit tests will
improve things - they are used already within efl.
> extensive tests for promise usage already, so having a good test case like
> this would improve the reliability of future changes to corresponding
> codepaths--which should not be too difficult given the time that you've
> already invested in debugging the issue to fully understand it.
since i understand it, i don't see what tests are going to do here that isn't
already done within efl.
> I'm constructing this mail from scratch since it looks like you pushed this
> from a branch without resetting the commit date and prevented a mail from
> being sent to the list, so please excuse any strange formatting in the
> patch diff below.
eh? ummm... nope. i did it from master. i had to do some jumping through
hoops with conflicts due to the revert breaking my fixes... ? i actually had
done the fix a few days earlier and was still testing/using it. i ended up
having to modify it due to the conflicts thus date moved forward a few days.
>
> Regards,
> Mike
>
> > commit de2ec0559b01dba7919503955cc47c1c5fcd0f97
> > Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com>
> > Date: Tue Dec 25 13:00:38 2018 +0000
> >
> > fix crashes created by "make efl_loop_promise_new a function"
> >
> > commit 9b5155c9f135f9ef450a817979f5884352b2d4c0 brought about crashes
> > - specifically that i saw in terminology because it actually uses
> > eina_promise_data_set() and the new efl_loop_promise_new basically
> > took over ownership of that data, but if anyone used
> > eina_promise_data_set() the data ptr used by this new code would bwe
> > overwritten, causing segfauls when terminology loses selection
> > ownership. for days i had mysterious crashes of terminology until i
> > narrowed it down to the above, so if you have too, then this will fix
> > it.
> >
> > what this does is create a data set intercept function callback that
> > for now is only for use inside efl to everride data sets so they set
> > data inside the new struct that tracks data. i also had to add and
> > intercept for eina_promise_data_free_cb_set() as this in theory could
> > also ber a similar problem.
> >
> > so perhaps the idea/design of efl_loop_promise_new() is not right and
> > this kind of thgn has to be internal to eina promise... this means
> > eina promise and loops are much more tied together.
> >
> > diff --git a/src/lib/ecore/efl_loop_consumer.c
> b/src/lib/ecore/efl_loop_consumer.c
> > index 29e94ed52d..1e49bc32aa 100644
> > --- a/src/lib/ecore/efl_loop_consumer.c
> > +++ b/src/lib/ecore/efl_loop_consumer.c
> > @@ -50,4 +50,68 @@ _efl_loop_consumer_future_rejected(Eo *obj,
> Efl_Loop_Consumer_Data *pd EINA_UNUS
> > return eina_future_rejected(efl_loop_future_scheduler_get(obj),
> error);
> > }
> >
> > +typedef struct _Efl_Loop_Consumer_Promise Efl_Loop_Consumer_Promise;
> > +struct _Efl_Loop_Consumer_Promise
> > +{
> > + EflLoopConsumerPromiseCancel func;
> > + Eina_Free_Cb free;
> > + void *data;
> > + Eo *obj;
> > +};
> > +
> > +static void
> > +_cancel_free(void *data)
> > +{
> > + Efl_Loop_Consumer_Promise *lcp = data;
> > +
> > + if (lcp->free) lcp->free(lcp->data);
> > + efl_unref(lcp->obj);
> > + free(lcp);
> > +}
> > +
> > +static void
> > +_cancel_triggered(void *data, const Eina_Promise *p)
> > +{
> > + Efl_Loop_Consumer_Promise *lcp = data;
> > +
> > + if (lcp->func) lcp->func(lcp->data, lcp->obj, p);
> > +}
> > +
> > +static void
> > +_data_set(Eina_Promise *p, void *data)
> > +{
> > + Efl_Loop_Consumer_Promise *lcp = eina_promise_data_get(p);
> > + lcp->data = data;
> > +}
> > +
> > +static void
> > +_cancel_free_cb_set(Eina_Promise *p, Eina_Free_Cb free_cb)
> > +{
> > + Efl_Loop_Consumer_Promise *lcp = eina_promise_data_get(p);
> > + lcp->free = free_cb;
> > +}
> > +
> > +static Eina_Promise *
> > +_efl_loop_consumer_promise_new(Eo *obj, Efl_Loop_Consumer_Data *pd,
> > + void *cancel_data,
> EflLoopConsumerPromiseCancel cancel, Eina_Free_Cb cancel_free_cb)
> > +{
> > + Efl_Loop_Consumer_Promise *lcp;
> > + Eina_Promise *p;
> > +
> > +
> > + lcp = calloc(1, sizeof (Efl_Loop_Consumer_Promise));
> > + if (!lcp) return NULL;
> > +
> > + lcp->func = cancel;
> > + lcp->data = cancel_data;
> > + lcp->free = cancel_free_cb;
> > + lcp->obj = efl_ref(obj);
> > +
> > + p = eina_promise_new(efl_loop_future_scheduler_get(obj),
> _cancel_triggered, lcp);
> > + eina_promise_data_set_cb_set(p, _data_set);
> > + eina_promise_data_free_cb_set_cb_set(p, _cancel_free_cb_set);
> > + eina_promise_data_free_cb_set(p, _cancel_free);
> > + return p;
> > +}
> > +
> > #include "efl_loop_consumer.eo.c"
> > diff --git a/src/lib/eina/eina_promise.c b/src/lib/eina/eina_promise.c
> > index c239ba6261..aa5ced8ab2 100644
> > --- a/src/lib/eina/eina_promise.c
> > +++ b/src/lib/eina/eina_promise.c
> > @@ -103,6 +103,8 @@ struct _Eina_Promise {
> > Eina_Future *future;
> > Eina_Future_Scheduler *scheduler;
> > Eina_Promise_Cancel_Cb cancel;
> > + Eina_Promise_Data_Set_Cb data_set_cb;
> > + Eina_Promise_Data_Free_Cb_Set_Cb data_free_cb_set_cb;
> > Eina_Free_Cb free_cb;
> > const void *data;
> > };
> > @@ -1116,7 +1118,14 @@ eina_promise_data_set(Eina_Promise *p,
> > void *data)
> > {
> > EINA_SAFETY_ON_NULL_RETURN(p);
> > - p->data = data;
> > + if (p->data_set_cb)
> > + {
> > + Eina_Promise_Data_Set_Cb cb = p->data_set_cb;
> > + p->data_set_cb = NULL;
> > + cb(p, data);
> > + p->data_set_cb = cb;
> > + }
> > + else p->data = data;
> > }
> >
> > EAPI void
> > @@ -1124,7 +1133,30 @@ eina_promise_data_free_cb_set(Eina_Promise *p,
> > Eina_Free_Cb free_cb)
> > {
> > EINA_SAFETY_ON_NULL_RETURN(p);
> > - p->free_cb = free_cb;
> > + if (p->data_free_cb_set_cb)
> > + {
> > + Eina_Promise_Data_Free_Cb_Set_Cb cb = p->data_free_cb_set_cb;
> > + p->data_free_cb_set_cb = NULL;
> > + cb(p, free_cb);
> > + p->data_free_cb_set_cb = cb;
> > + }
> > + else p->free_cb = free_cb;
> > +}
> > +
> > +EAPI void
> > +eina_promise_data_set_cb_set(Eina_Promise *p,
> > + Eina_Promise_Data_Set_Cb data_set_cb)
> > +{
> > + EINA_SAFETY_ON_NULL_RETURN(p);
> > + p->data_set_cb = data_set_cb;
> > +}
> > +
> > +EAPI void
> > +eina_promise_data_free_cb_set_cb_set(Eina_Promise *p,
> > + Eina_Promise_Data_Free_Cb_Set_Cb
> data_free_cb_set_cb)
> > +{
> > + EINA_SAFETY_ON_NULL_RETURN(p);
> > + p->data_free_cb_set_cb = data_free_cb_set_cb;
> > }
> >
> > static Eina_Value
> > diff --git a/src/lib/eina/eina_promise.h b/src/lib/eina/eina_promise.h
> > index 1896260dba..060d8dccf8 100644
> > --- a/src/lib/eina/eina_promise.h
> > +++ b/src/lib/eina/eina_promise.h
> > @@ -187,6 +187,26 @@ struct _Eina_Future_Scheduler {
> > */
> > typedef void (*Eina_Promise_Cancel_Cb) (void *data, const Eina_Promise
> *dead_promise);
> >
> > +/*
> > + * @typedef Eina_Promise_Data_Set_Cb Eina_Promise_Data_Set_Cb.
> > + * @ingroup eina_promise
> > + *
> > + * A callback used to intercept eina_promise_data_set().
> > + *
> > + * Used internally by EFL - please do not use.
> > + */
> > +typedef void (*Eina_Promise_Data_Set_Cb) (Eina_Promise *p, void *data);
> > +
> > +/*
> > + * @typedef Eina_Promise_Data_Free_Cb_Set_Cb
> Eina_Promise_Data_Free_Cb_Set_Cb.
> > + * @ingroup eina_promise
> > + *
> > + * A callback used to intercept eina_promise_data_set_cb_set().
> > + *
> > + * Used internally by EFL - please do not use.
> > + */
> > +typedef void (*Eina_Promise_Data_Free_Cb_Set_Cb) (Eina_Promise *p,
> Eina_Free_Cb free_cb);
> > +
> > /**
> > * @typedef Eina_Future_Success_Cb Eina_Future_Success_Cb.
> > * @ingroup eina_future
> > @@ -636,6 +656,28 @@ EAPI void eina_promise_data_set(Eina_Promise *p,
> void *data) EINA_ARG_NONNULL(1)
> > */
> > EAPI void eina_promise_data_free_cb_set(Eina_Promise *p, Eina_Free_Cb
> free_cb);
> >
> > +/**
> > + * Sets a data set intercept function that can alter the behavior of
> > + * eina_promise_data_set(). Please do not use this as it is only used
> > + * internally inside EFL and may be used to slightly alter a promise
> > + * behavior and if used on these promises may remove EFL's override
> > + *
> > + * @param[in] p The promise to set the data set callback on
> > + * @param[in] data_set_cb The callabck to intercept the data set
> > + */
> > +EAPI void eina_promise_data_set_cb_set(Eina_Promise *p,
> Eina_Promise_Data_Set_Cb data_set_cb);
> > +
> > +/**
> > + * Sets a data free cb set intercept function that can alter the
> behavior of
> > + * eina_promise_data_free_cb_set(). Please do not use this as it is only
> used
> > + * internally inside EFL and may be used to slightly alter a promise
> > + * behavior and if used on these promises may remove EFL's override
> > + *
> > + * @param[in] p The promise to set the data set callback on
> > + * @param[in] data_free_cb_set_cb The callabck to intercept the data
> free cb set
> > + */
> > +EAPI void eina_promise_data_free_cb_set_cb_set(Eina_Promise *p,
> Eina_Promise_Data_Free_Cb_Set_Cb data_free_cb_set_cb);
> > +
> > /**
> > * Resolves a promise.
> > *
>
> _______________________________________________
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
--
------------- Codito, ergo sum - "I code, therefore I am" --------------
Carsten Haitzler - ras...@rasterman.com
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
