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

[George Sedov <radist morse gmail com>]

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.
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()
3. The methods that do require OS on the other hand, like
frl.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.

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.
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.
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

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:

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

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