Arf, this didn't got to the ml...
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On April 23, 2018 3:27 PM, Cedric Bail <ced...@ddlm.me> wrote:
> On April 21, 2018 7:46 AM, Carsten Haitzler ras...@rasterman.com wrote:
> > On Wed, 18 Apr 2018 12:38:46 -0400 Cedric Bail ced...@ddlm.me said:
> > > On April 18, 2018 12:48 AM, Carsten Haitzler ras...@rasterman.com wrote:
> > > > On Tue, 17 Apr 2018 19:33:02 -0400 Cedric Bail ced...@ddlm.me said:
> > > > > On March 29, 2018 10:58 AM, Carsten Haitzler ras...@rasterman.com
> > > > > wrote:
> > > > > > On Thu, 29 Mar 2018 13:17:43 -0400 Cedric Bail ced...@ddlm.me said:
> > > > > > > On March 28, 2018 8:06 PM, Carsten Haitzler ras...@rasterman.com
> > > > > > > wrote:
> > > > > > >
> > > > > > > > On Wed, 28 Mar 2018 12:37:35 -0400 Cedric Bail ced...@ddlm.me
> > > > > > > > said:
> > > > > > > > > On March 27, 2018 10:32 PM, Carsten Haitzler
> > > > > > > > > ras...@rasterman.com
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > On Tue, 20 Mar 2018 17:57:47 -0700 Cedric BAIL
> > > > > > > > > > cedric.b...@free.fr said:
> > > > > > > > > >
> > > > > > > > > > Ideally yes, but at least for the _example, it requires to
> > > > > > > > > > spend some more time to move them to use EFL_MAIN, which
> > > > > > > > > > should
> > > > > > > > > > be done in a different patch not related to fixing the
> > > > > > > > > > usage of
> > > > > > > > > > efl_add.
> > > > > > > >
> > > > > > > > some use EFL_MAIN ... otherwise efl_main_loop_get() will work.
> > > > > > > > but
> > > > > > > > moving to efl_add_ref() isn't the "right direction" for most of
> > > > > > > > these.
> > > > > > >
> > > > > > > Yeah, I haven't been a big fan of efl_main_loop_get and did tend
> > > > > > > to
> > > > > > > avoid it. I should use it more often in the future.
> > > > > >
> > > > > > it'd be good to stop doing that... :) long term at least. the slow
> > > > > > and
> > > > > > steady steps are to fix our parenting in efl and examples. :) i
> > > > > > fixed it
> > > > > > for you - or well i fixed everything i saw you changed. it may not
> > > > > > be
> > > > > > perfect, but it's a step in the right direction imho :) point out
> > > > > > if i
> > > > > > got something wrong. i didn't do "Extensive" work like you say and
> > > > > > convert to EFL_MAIN or within efl modify code to pass parents around
> > > > > > many levels down or something.
> > > > >
> > > > > So I have been working on finishing correcting the lifecycle of eo
> > > > > object
> > > > > by making sure that efl_invalidate does happen before any
> > > > > efl_destructor
> > > > > code (Currently in master, it is possible to get efl_invalidate to be
> > > > > called after the object destructor as it is called inside
> > > > > efl.object.destructor). This means that efl_del and
> > > > > efl_parent_{get,set}
> > > > > are a typical wrong piece of code in the destructore. The patch
> > > > > 2fb5cc3ad09f6aaf82b5d1131ac5ed22ed848bd4 is wrong in a lot of place
> > > > > that
> > > > > I had to give up fixing it due to time constraint (basically it assume
> > > > > our use before invalidate was efl_add/efl_del, while the closest to
> > > > > our
> > > > > pattern was efl_add_ref with efl_unref in the desstructor). I have a
> > > > > branch devs/cedric/lifecycle with all the change in it including a
> > > > > revert
> > > > > of your patch. I can't land this patch at the moment as the old eo
> > > > > base
> > > > > efl_future lifecycle is a complete wreck with this change, so I will
> > > > > focus back on removing efl_future. If you had time in the mean time to
> > > > > check your patch and see how to fix it, it would be great.
> > > >
> > > > wait wait. first. some objects. actually a fair few objects need to know
> > > > their parents to find their loop provider etc. an they need to do this
> > > > on
> > > > "shut down" to delete stuff etc. from here. so my question will be:
> > > > where should an object do this "shut down" in the invalidate or in
> > > > destructor. you seem to say that it mys be in invalidate which means
> > > > invalidate effectifely is the "meat" of the destructor, just the object
> > > > keeps itself around with some minimal content until destructor is
> > > > called.
> > > > so what you are doing is ensuring there is ALWAYS the sequence of
> > > > invalidate then destroy, right?
> > >
> > > So yes, invalidate should always happen before the destructor.
> > >
> > > > so i'm not sure what's wrong with that patch - can you detail it? i
> > > > simply
> > > > moved to having a parent instead of no parent in many cases, and used
> > > > efl_del again like it used to be which should clear out the only
> > > > reference
> > > > to the object by unparenting it etc. etc. ...
> > >
> > > The problem is that efl_del in a destructor will always be either wrong or
> > > bad. Wrong as in you are not the parent/owner of that reference and are
> >
> > wait a sec... at least at the time of the patches in question here, efl was
> > still basically all using destructor only. no invalidate existed (or was
> > used).
> > it's still basically not used except for canvas objects, vg node objects,
> > base
> > class, smart layout objects, model containers and efl app. so let's say its
> > essentially not used. the invalidate stuff was put into the efl tree after
> > the
> > patches in question here, so at least my partial revert of your changes
> > (back
> > to using parents and efl del) was correct at the time, or as correct as was
> > possible. is this correct? was there anything wrong at that stage?
>
> invalidate already existed when that patch landed. It was the entire reason
> why Mike helped me land this big patch in the first place as we needed to
> handle life cycle of all object correctly. It is not necessary to implement
> invalidate on an object, like it is not necessary to always implement a
> destructor or a constructor or finalize, it doesn't mean that the object will
> not go through the invalidate state. So your patch basically move from
> manually managing lifecycle (efl_add_ref/efl_unref) to kind of automatically
> managed lifecycle with efl_add, but to much efl_del.
>
> You patch is not correct at the time it was pushed if for any of the object
> you changed you do the following sequence :
>
> efl_ref(obj);
> efl_del(obj);
> efl_unref(obj);
>
> You will have in that scenario a lot of dead pointer being manipulated during
> the destructor which will be called during efl_unref. Arguably none of our
> tests does check that and you have to go dig around to trigger them in our
> code base. The only reason why we don't see the problem everywhere is that
> right now, if you do efl_del on an object, it first call the custom object
> destructor first, invalidate the object and finally call
> efl_object_destructor. Which is not correct as invalidate should be the last
> point in time where you parent is usable. So calling it during the destructor
> sequence result to case where the parent will be NULL during invalidate,
> which is a problem that needs fixing.
>
> > now calling efl_del in a destructor... i am unsure why it would often or
> > always
> > be wrong? if you are going to manually destroy some object (a child or some
> > other linked object). at least in a world where invalidate is essentially
> > unused (see above) then the only place it can happen is in the
> > destructor... my
> > reading of efl_object_invalidate in the base class is that it will clear
> > futures and unhook the parent of the object but do nothing to children, so
> > that
> > means children have not been deleted yet as you say below... as the only
> > efl_del()'s in a destructor are for child objects OR for linked objects that
> > are not actually children but need deletion anyway... so i'm still
> > confused. :(
>
> If you create your child object with efl_add, the only reference that keep
> them alive is the parent reference. When an object is about to loose its
> parent and get invalidated, all its children have to be invalidated first as
> otherwise they would still believe they can expect meaningful things from
> their parent tree which they can't if there parent are invalidated. This
> means that all children will loose their reference to their parent, which is
> the reference that was keeping them. So when reaching the destructor stage,
> they are dead if they were created with efl_add and as efl_del imply that you
> are destroying the parent relationship, it makes no sense to be done in the
> destructor.
>
> In general, if you do an efl_add, you do not need to do an efl_del as the
> goal is to get the object to be destroyed when the parent is destroyed.
> Previous code was working because we were manually managing the life cycle of
> the object with efl_add_ref and so the object would still be alive during
> destructor. If you need the object to be alive during the destructor that is
> the way to go. Ofcourse relying on the parent relationship to manage the
> destruction of the object is best and reduce the amount of code that needs to
> be written, so having a proper patch that does that would be the best.
>
> > > trying to destroy it manually during destructor. Bad as in the child
> > > object
> > > has already lost its parent during the invalidate call on the parent, and
> > > if
> >
> > ummm how can this happen:
> >
> > static void
> > _efl_object_invalidate(Eo *obj, Efl_Object_Data *pd)
> > {
> > _efl_pending_futures_clear(pd);
> > efl_parent_set(obj, NULL);
> > pd->invalidate = EINA_TRUE;
> > }
> >
> > the PARENT obj (obj above) loses its parent, and if it has efl_del in the
> > destructor of obj that deletes other child objects of "obj" i don;'t see how
> > these are bad/wrong... so as above. i'm still confused. the invalidate on
> > obj
> > doesn't iterate over children invalidating them too... at least my quick
> > read of
> > the code doesn't say that... if it did then you'd be right.
>
> As explained above and said before, current behavior in master is not totally
> correct in all situation. It is corrected in my branch, which I am not
> landing until we figure this problem along with a few others.
>
> > > it is the only reference you had on it, it will be dead when reaching the
> > > destructor. So during destructor calling efl_del is likely accessing a
> > > dead
> > > object. Basically if we do efl_add, we should not need to call efl_del in
> > > the
> > > majority of case, just NULL the reference in the structure during
> > > invalidate,
> > > that's it. It does simplify the code a lot actually, but it differ from
> > > the
> > > code pattern we have and require more work than just replacing
> > > efl_add_ref/efl_unref by efl_add/efl_del.
> >
> > ummm it would make sense if the children were iterated over and
> > invalidated...
> > it's not what i see, so... ummm i'm confused. :)
>
> My branch is devs/cedric/lifecycle.
>
> > > > if you reverted the patch you would have lost all the "correct"
> > > > parenting so
> > > > i'm not sure that that is going to be better at all, unless you kept
> > > > that
> > > > and it's a partial revert really. i haven't looked at your branch yet,
> > > > but
> > > > it's kind of not making a lot of sense in this email right now. i am not
> > > > sure what needs fixing where and how from this email at all, so it'd be
> > > > nice for you to explain some more...
> > >
> > > In most case where efl_add_ref was used with efl_unref, the parenting was
> > > not
> > > a problem and the code was working. It was not optimally using eo
> > > capability
> > > for sure.
> >
> > but since it needed to be done, it's now done anyway. :) so it's probably
> > not
> > worth undoing that, but instead looking at this del problem you say we have,
> > but i can't seem to grasp... unless you've changed the above in your branch
> > and
> > i should dig around there (the invalidate on base class iterates over
> > children
> > invalidating them for example)?
>
> It was fairly more trivial to revert it (in my branch) than to actually go
> through fixing all the problem it created. And yes, you should dig in my
> branch :-)
>
> Cedric
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
