Page MenuHomePhabricator

efl: improve Efl.Container_Model test to have proper lifecycle.
ClosedPublic

Authored by cedric on Feb 1 2019, 4:00 AM.

Diff Detail

Repository
rEFL core/efl
Branch
T7528-devs/cedric/child_noref
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9639
cedric created this revision.Feb 1 2019, 4:00 AM
cedric requested review of this revision.Feb 1 2019, 4:00 AM
segfaultxavi requested changes to this revision.Feb 1 2019, 8:25 AM

Hmmm... you either efl_add_ref() and then efl_del() at the end, or you only efl_add(), no?
Aren't you unreffing too much now?

This revision now requires changes to proceed.Feb 1 2019, 8:25 AM
cedric added a comment.Feb 4 2019, 1:06 PM

Hmmm... you either efl_add_ref() and then efl_del() at the end, or you only efl_add(), no?
Aren't you unreffing too much now?

It is indeed not necessary to do the efl_del as the destruction of the main loop object would actually do the equivalent of the efl_del. Still moving to efl_add is better as there is no additional reference that require efl_unref at the end and I do like to have the symetrical efl_add/efl_del, but if you prefer to get the efl_del removed, I will do.

OK, I understand you use efl_del() to unref the model and remove it from the main loop, so it is destroyed completely at this point.
However, I do not think we should be using the return value of efl_add() at all. Some time ago, we even considered that efl_add() should return void to avoid confusion!
Given that you need to use the newly-created object, you must use efl_add_ref() and dispose of the extra ref at the end. No 100% safe way around it.

Otherwise, I'll have to change the Eo tutorial.

cedric added a comment.Feb 5 2019, 9:40 AM

OK, I understand you use efl_del() to unref the model and remove it from the main loop, so it is destroyed completely at this point.
However, I do not think we should be using the return value of efl_add() at all. Some time ago, we even considered that efl_add() should return void to avoid confusion!
Given that you need to use the newly-created object, you must use efl_add_ref() and dispose of the extra ref at the end. No 100% safe way around it.

That's weird in this context as this would mean I would have to do efl_unref and efl_del. The issue is when using the main loop as parent for an object, you do not control the parent even if you rely on it. It is kind of broken case compared to the normal case of when you can use efl_add and you actually are the parent. So you end up setting the main loop as parent and the parent has a reference on the object. You know that the main loop is not going to be destroyed until the call to shutdown, so you do not really need to have an additional reference for the scope you are in. You just need to disconnect the object from the main loop to also destroy. It is kind of an abuse of the system, but feels better to me than to do : efl_add_ref, efl_unref, efl_del in the same function.

Otherwise, I'll have to change the Eo tutorial.

I think this tutorial is correct, it is just because of the use of the main loop object as parent here, that there is some possible shortcut (aka, no need to take an additional reference for the scope as it is guaranteed that the main loop will exist from init to shutdown).

segfaultxavi accepted this revision.Feb 6 2019, 3:37 AM

That's weird in this context as this would mean I would have to do efl_unref and efl_del.

Oh, I always assumed efl_del() removed TWO references when an object had a parent, but I checked the code and I was wrong. Ugh, sorry.

I understand why you feel comfortable using a reference you do not own, because you know the parent (the main loop in this case) won't disappear any time soon.
What I fight every day for you guys to understand is that, as I user, I do not know that. In my imagination, the main loop could be a different thread which could disappear at any time, and take with it all its references.
To solve that, we can 1) explicitly document that the main loop is a special object which will always be there for you (sure? what if I am a worker thread and the main loop is being shutdown on the main thread?).
Or 2) we can explain the users a simple golden rule: Only use objects when you have a reference to them (as every other lib out there does).

Now, I understand this is a test; the object is created and destroyed in the same method, and it would definitely be awkward to have efl_unref() followed by efl_del(). So yeah, leave it as it is now.

Just don't use this approach very often, or you will be confusing our readers about a very delicate topic :)

Also, I just realized this patch only affects a test file, you better write that in the commit title!

This revision is now accepted and ready to land.Feb 6 2019, 3:37 AM
cedric added a comment.Feb 7 2019, 1:11 AM

That's weird in this context as this would mean I would have to do efl_unref and efl_del.

Oh, I always assumed efl_del() removed TWO references when an object had a parent, but I checked the code and I was wrong. Ugh, sorry.

That was actually the case a year ago, before I fixed it. We had efl_del either remove one or two reference depending on how the wind was going. Now efl_del only remove the reference to the parent and that is it.

I understand why you feel comfortable using a reference you do not own, because you know the parent (the main loop in this case) won't disappear any time soon.
What I fight every day for you guys to understand is that, as I user, I do not know that. In my imagination, the main loop could be a different thread which could disappear at any time, and take with it all its references.
To solve that, we can 1) explicitly document that the main loop is a special object which will always be there for you (sure? what if I am a worker thread and the main loop is being shutdown on the main thread?).

At this point, I don't think we have a workable model for thread at all. So in that regard, for the duration of a program, as long as the user can execute code, their is going to be a reference on the main loop that will remain available. Only the shutdown of efl will actually lead to the destruction of the main loop.

Or 2) we can explain the users a simple golden rule: Only use objects when you have a reference to them (as every other lib out there does).

In which case the above code should actually efl_ref/efl_unref the main loop. I would actually find that more correct and making sense.

Now, I understand this is a test; the object is created and destroyed in the same method, and it would definitely be awkward to have efl_unref() followed by efl_del(). So yeah, leave it as it is now.

Just don't use this approach very often, or you will be confusing our readers about a very delicate topic :)

Also, I just realized this patch only affects a test file, you better write that in the commit title!

cedric updated this revision to Diff 19239.Feb 7 2019, 1:57 AM
cedric retitled this revision from efl: improve Efl.Container_Model to have proper lifecycle. to efl: improve Efl.Container_Model test to have proper lifecycle..

Rebase and reword.

cedric updated this revision to Diff 19564.Thu, Feb 21, 2:06 PM

Rebase.

cedric updated this revision to Diff 19567.Thu, Feb 21, 2:42 PM

Rebase.

cedric updated this revision to Diff 19571.Thu, Feb 21, 6:01 PM

Rebase.

This revision was automatically updated to reflect the committed changes.