GNOME.org
 

Re: lua-factory: the analysis of the changes proposed in #753141

Hi,

First of all, thanks for your time and work on this.
Comments bellow.

On Wed, Oct 21, 2015 at 06:26:19PM +0300, George Sedov wrote:

Hi,

So there was some confusion regarding the bug #753141, and what was the
ideas behind the proposed changes. Let me explain the situation.

Right now, the lua plugins are working this way:

1. When the plugin is asked to perform the operation, lua-factory
constructs the structure named OperationSpec (OS), in where it puts all
the relevant information about the operation, most of it taken directly
from the GrlSourceSearchSpec, BrowseSpec and such.

2. The constructed OS is then put into the lua context, into the global
table "_G" with the unique key, constructed from the operation_id (see
https://git.gnome.org/browse/grilo-plugins/tree/src/lua-factory/grl-lua
-library.c#n1584)

3. After putting the OS, lua-factory also puts the same data into the
same global table, only this time with the key, which is the same for
all the operations, and with that, that operation is considered
"current" (see https://git.gnome.org/browse/grilo-plugins/tree/src/lua-
factory/grl-lua-library.c#n1658). This function also does the check for
the "unfinalized operations" which I will explain later.

3. As part of it's lua API, lua-factory provides several functions that
use the OS data. Most of them are synchronous and just take the OS
which is "current", but there are two of them that require callbacks,
namely grl.fetch and grl.unzip. They save the operation_id of the
"current" operation internally, and use it later in the callback to
retrieve the relevant OS from the global table. They retrieve it by
making their operation "current" again, and after they are done, they
invalidate the "current" operation completely. These function also
increase the special counter of unfinished async calls, to keep the
track of when every particular operation is really "done".

4. Now about this "unfinalized operation" check. The idea behind it is,
that by the time the next operation starts, all the synchronous calls
from the previous operation have already been finished. So every OS
that has the counter of unfinished async calls = 0 should be
"finalized", i.e. the finalizing grilo callback must be called. If it's
not, it is called forcibly with the warning of "broken plugin" being
made.

Apart from the obvious over-complication, this approach has several
flaws.

1. The foremost: this approach will totally not work in case of several
operations being run simultaneously. There can be only one "current"
operation, and so if the previous operation didn't finish it's
synchronous part by the time the next operation started, it is
finalized forcibly anyway. Another screw-up will happen if several lua
async callback would be run simultaneously. They will rewrite the
"current" operation and reset it when the first of them would be done.
All in all, the design is absolutely async-unfriendly.


All in all, I agree with you. It is over-complicated but I don't see how
it could break with several operations from grilo (like, several async
search/browse) as each OperationSpec stays in _G related to its
operation-id. If you *can* break it easily, tell me how or even better,
create a test under lua-factory [1] and attach to the BZ :-)

[1] https://git.gnome.org/browse/grilo-plugins/tree/tests/lua-factory

Also, what I think is "async-unfriendly" is the "string" as callback
without any argument as user_data. We workaround this with a global
table in the plugin which make it ugly IMHO. Bad design.


2. The async methods like grl.fetch can't work without OS, even though
they don't require them directly. So you can't fetch anything during
e.g. grl_source_init()


Agreed. Bad design.


3. The methods that do require OS on the other hand, like
grl.get_options, can be called from everywhere, even though it does
nothing and doesn't make any sense outside of the scope of the
respective operation.



So, the idea is provide all functions under 'grl' table. This is not
strange at all and keep everything very simple to the developer.
If a function does not make sense to be called then it should not be
called - It does not mean that we must control when developer use it.

grl.get_options was indeed created thinking that would be called inside
an operation handler - thus it would fail otherwise as it would not be
able to get OperationSpec. /* broken plugin */


And now, after we refreshed the current situation, the changes proposed
in the patches from #753141

1. Remove the OS from the global tables, as well as the concept of the
"current" operation.


I'm okay with this.


2. Similarly, remove all the functions that require OS directly from
the global scope. There are actually only four of them: grl.get_options
grl.get_requested_keys grl.get_media_keys and grl.callback.


Right.


3. Now comes the tricky part. To be able to access the OS from inside
these functions, the C closures are constructed for every such
functions with OS as an upvalue, and these closures are then passed to
the lua functions as parameters. From the lua code this will look like
this:

was:

function grl_source_search (query)
    local count = grl.get_options("count")
    grl.callback()
end

become:

function grl_source_search (query, options, callback)
    local count = options("count")
    callback()
end



So, this changes a lot. What I like least is that you are splitting
functions provided by grl-lua-library. I do like grl as a library -
globally accessible.

I might need some studying to closures to realize why this would be a
bad idea but why not having the whole grl table as closure then?

Also: would it be worse if we set `grl.get_options = options` before
calling the operation handler? even if get_options and friends are not
accessible afterwards due gc cleanup, that might be better as we keep
functions centralized.

** aside of `callback` which is the only one I think it makes sense to
keep as an argument here.


4. Since all the methods that require the access to OS should be passed
to the lua functions as parameters, it is best to make the number of
these methods as few as possible. That's why the methods
grl.get_options grl.get_requested_keys and grl.get_media_keys were
merged into one. otherwise the function would look something like

function grl_source_search (query, options, requested_keys, media_keys,
callback)

5. As a final step, the way the lua async functions are working was
changed.

was:

function async_callback (data)
end

function grl_source_search (query)
    grl.fetch("something","async_callback")
end

become:

function async_callback (data)
end

function grl_source_search (query)

grl.fetch("something",async_callback)
end

And if it doesn't strike you as a big difference, then consider these
possibilities:



Yes, this is much better indeed.


grl.fetch("something",function(data) print data end)

or

local userdata = {}
grl.fetch("something",function(data) async_callback(data, userdata) end)

There is also provided an option of passing the userdata directly

function async_callback (data, userdata)
end

function grl_source_search (query)
    grl.fetch("something",async_callback, {}, userdata)
end

The extra {} is required as an option for grl.fetch



It is optional, not really required.


So, these are the changes that are proposed in bug #753141. As you can
see, they are quite API breaking, no way around it, but I think the
benefits are worth it.

Cheers,
      George


I have looked into the patches and vk source while reading/replying
this email. I hope to do a proper review soon.

  toso
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]