GNOME Bugzilla – Bug 658272
Port gtksearchenginetracker.c to tracker 0.11/0.12
Last modified: 2011-09-13 01:36:04 UTC
At the same time, we should evaluate dropping the dlopen hack and just using trackers dbus interface.
This should be really straight forward. There are plenty of examples¹ and tools² to steal examples from: ¹ http://git.gnome.org/browse/tracker/tree/examples/libtracker-sparql/async-connection.c ² http://git.gnome.org/browse/tracker/tree/src/tracker-utils/tracker-search.c ... The above doesn't use *just* D-Bus but it should be quite easy to implement.
If 'just use dbus' is not the best way forward, I would be willing to consider just linking against trackers libraries, instead of the dlopen hack
Created attachment 195928 [details] [review] First attempt at fixing this This is just something I knocked up this afternoon. It should be on the right tracks. Things to note which we had to do to avoid linking with Tracker: - Import tracker_sparql_escape_string() - Import tracker_string_list_to_gslist() (though for GList for the hits) Right now it works quite nicely, I have a few open questions though: - Why isn't the object ever finalised? Looks like a memory leak - Is the raw D-Bus approach fine or do we prefer to use GDBusProxy? - We don't watch for connection drops, we just check each time - could be optimised further perhaps, not sure it's necessary. - The while (gtk_events_pending () code is not too nice, is this acceptable? I wanted to avoid blocking the UI if Tracker is replaying the journal or delaying the response to the Wait() method. Current debug output is minimal and looks like this: (lt-gtk3-demo:24167): Gtk-DEBUG: -- (lt-gtk3-demo:24167): Gtk-DEBUG: Finding out if Tracker is available via D-Bus... (lt-gtk3-demo:24167): Gtk-DEBUG: Tracker is ready (lt-gtk3-demo:24167): Gtk-DEBUG: Creating GtkSearchEngineTracker... (lt-gtk3-demo:24167): Gtk-DEBUG: Query starting, search criteria:'foo', location:'(null)' (lt-gtk3-demo:24167): Gtk-DEBUG: Found 5 hits (lt-gtk3-demo:24167): Gtk-DEBUG: Query stopping (lt-gtk3-demo:24167): Gtk-DEBUG: -- (lt-gtk3-demo:24167): Gtk-DEBUG: Finding out if Tracker is available via D-Bus... (lt-gtk3-demo:24167): Gtk-DEBUG: Tracker is ready (lt-gtk3-demo:24167): Gtk-DEBUG: Creating GtkSearchEngineTracker... (lt-gtk3-demo:24167): Gtk-DEBUG: Query starting, search criteria:'bar', location:'(null)' (lt-gtk3-demo:24167): Gtk-DEBUG: Found 17 hits (lt-gtk3-demo:24167): Gtk-DEBUG: Query stopping (lt-gtk3-demo:24167): Gtk-DEBUG: -- (lt-gtk3-demo:24167): Gtk-DEBUG: Finding out if Tracker is available via D-Bus... (lt-gtk3-demo:24167): Gtk-DEBUG: Tracker is ready (lt-gtk3-demo:24167): Gtk-DEBUG: Creating GtkSearchEngineTracker... (lt-gtk3-demo:24167): Gtk-DEBUG: Query starting, search criteria:'baz', location:'(null)' (lt-gtk3-demo:24167): Gtk-DEBUG: Found 1 hits (lt-gtk3-demo:24167): Gtk-DEBUG: Query stopping -- OR -- (lt-gtk3-demo:26391): Gtk-DEBUG: Finding out if Tracker is available via D-Bus... (lt-gtk3-demo:26391): Gtk-DEBUG: Tracker is not available, GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.Tracker1 was not provided by any .service files -- Comments?
(In reply to comment #2) > If 'just use dbus' is not the best way forward, I would be willing to consider > just linking against trackers libraries, instead of the dlopen hack While we could do that, a few of us on the #tracker channel (Carlos, Cosimo, Jürg) agreed it makes sense to avoid linking with Tracker. The current patch works nicely so I am quite happy with the approach we all agreed on there.
Hmm, the tabs/spacing in the patch are all broken for some reason - looks fine in emacs here - I will try to fix this tomorrow.
Created attachment 196036 [details] [review] Updated with the correct coding style
Review of attachment 196036 [details] [review]: ::: gtk/gtksearchenginetracker.c @@ +41,3 @@ +#define DBUS_INTERFACE_STATUS "org.freedesktop.Tracker1.Status" + + seconds. @@ +44,3 @@ +#define WAIT_TIMEOUT 1 + +#define DBUS_PATH_STATUS "/org/freedesktop/Tracker1/Status" seconds. @@ +119,3 @@ + } + + result, that should be an else branch for: if (reply) @@ +124,3 @@ + error->message); + g_error_free (error); + if (reply) Magic numbers. Use an enum, or defines instead. @@ +187,3 @@ + + /* Wait */ + DBUS_INTERFACE_STATUS, That's very very likely a bad idea. @@ +240,1 @@ if (error) Again, should be the "else" statement.
(In reply to comment #3) > - The while (gtk_events_pending () code is not too nice, is this acceptable? > I wanted to avoid blocking the UI if Tracker is replaying the journal or > delaying the response to the Wait() method. It will most probably crash the app, if the user decides to close the file-chooser dialog while you are waiting for the reply from dbus; the file-chooser is probably not prepared to be destroyed in it's constructor (which is I guess something you need to expect as a reaction to a "close" event). I guess you'll either need to choose a faster way to detect whether tracker is available as a backend, or if that is not possible, do it properly asynchronous. Maybe it's reasonable to assume that the "simple" search is always available as fall-back, and thus the decision upon which search engine is used can actually be postponed until the user has typed some search-string. So it could instantiate both the tracker and simple backends and if tracker has responded until the user starts the search, that is used, otherwise the simple-fallback. Just 2¢.
(In reply to comment #7) > Review of attachment 196036 [details] [review]: > > ::: gtk/gtksearchenginetracker.c > @@ +41,3 @@ > +#define DBUS_INTERFACE_STATUS "org.freedesktop.Tracker1.Status" > + > + > > seconds. Not actually sure what you're saying here? > @@ +44,3 @@ > +#define WAIT_TIMEOUT 1 > + > +#define DBUS_PATH_STATUS "/org/freedesktop/Tracker1/Status" > > seconds. You would prefer WAIT_TIMEOUT_SECONDS ? > @@ +119,3 @@ > + } > + > + result, > > that should be an else branch for: > if (reply) Why? If there is (no|a) reply and there is no error, it's a success. If there is (no|a) reply and there is an error, it falls into the if (error) condition below, it's a failure. There is no way retval can not be set given we get some response. Not sure why we need an else? > @@ +124,3 @@ > + error->message); > + g_error_free (error); > + if (reply) > > Magic numbers. Use an enum, or defines instead. Yea, thought about it. Probably right - I wasn't even sure of this approach. The alternative was a mainloop and I figured it would be less appealing. > @@ +187,3 @@ > + > + /* Wait */ > + DBUS_INTERFACE_STATUS, > > That's very very likely a bad idea. Care to elaborate? > @@ +240,1 @@ > if (error) > > Again, should be the "else" statement. Why? Else if perhaps with the following condition, but else on it's own is just putting the rest of the function indented. -- Thanks for the comments Bastien.
Martyn, the code snippets from the review appear to be messed up in the Bugzilla comment. It looks fine in Splinter's review UI, though.
(In reply to comment #9) > (In reply to comment #7) > > Review of attachment 196036 [details] [review] [details]: > > > > ::: gtk/gtksearchenginetracker.c > > @@ +41,3 @@ > > +#define DBUS_INTERFACE_STATUS "org.freedesktop.Tracker1.Status" > > + > > + > > > > seconds. > > Not actually sure what you're saying here? Read it in the review panel, it borked my comment (it's in the comments, you say "time in second", it's "seconds"). > > @@ +44,3 @@ > > +#define WAIT_TIMEOUT 1 > > + > > +#define DBUS_PATH_STATUS "/org/freedesktop/Tracker1/Status" > > > > seconds. > > You would prefer WAIT_TIMEOUT_SECONDS ? As above. > > @@ +119,3 @@ > > + } > > + > > + result, > > > > that should be an else branch for: > > if (reply) > > Why? > > If there is (no|a) reply and there is no error, it's a success. > If there is (no|a) reply and there is an error, it falls into the if (error) > condition below, it's a failure. > > There is no way retval can not be set given we get some response. Not sure why > we need an else? You do "if (error)", that's not the way to check for errors. You're supposed to check the retval of the function instead. > > @@ +187,3 @@ > > + > > + /* Wait */ > > + DBUS_INTERFACE_STATUS, > > > > That's very very likely a bad idea. > > Care to elaborate? See comment 8, it's clearly not acceptable. > > @@ +240,1 @@ > > if (error) > > > > Again, should be the "else" statement. > > Why? Else if perhaps with the following condition, but else on it's own is just > putting the rest of the function indented. As above.
(In reply to comment #11) > You do "if (error)", that's not the way to check for errors. You're supposed to > check the retval of the function instead. Is there a policy somewhere I've not read about this? AFAIK, if the return value is not as expected, we do expect error to be set (if given) in such cases. This argument is only relevant if error can be unset while the reply is not expected. > > > That's very very likely a bad idea. > > > > Care to elaborate? > > See comment 8, it's clearly not acceptable. I know it's not the best approach because I mentioned it in my opening comments (and I am also responding to comment 8 separately) :) I wanted to know the right way or your suggestion here ;)
(In reply to comment #8) > (In reply to comment #3) > > - The while (gtk_events_pending () code is not too nice, is this acceptable? > > I wanted to avoid blocking the UI if Tracker is replaying the journal or > > delaying the response to the Wait() method. > > It will most probably crash the app, if the user decides to close the > file-chooser dialog while you are waiting for the reply from dbus; the > file-chooser is probably not prepared to be destroyed in it's constructor > (which is I guess something you need to expect as a reaction to a "close" > event). I just tested this. It doesn't crash if you close the dialog before a reply is received. Additionally, the current tracker searchengine implementation use tracker_sparql_connection_get() which does Wait() internally anyway. > I guess you'll either need to choose a faster way to detect whether tracker is > available as a backend, or if that is not possible, do it properly > asynchronous. Detecting tracker is available is not the problem, the Wait() makes sure that the database is ready to be used. There are cases where we replay the journal or need to apply any schema changes and that means the resources are not available immediately. > Maybe it's reasonable to assume that the "simple" search is always available as > fall-back, and thus the decision upon which search engine is used can actually > be postponed until the user has typed some search-string. That does sound reasonable. I would work that in. > So it could > instantiate both the tracker and simple backends and if tracker has responded > until the user starts the search, that is used, otherwise the simple-fallback. AFAICS, it will only try the next one if the previous implementation returns NULL. So that's not possible.
(In reply to comment #8) > Maybe it's reasonable to assume that the "simple" search is always available as > fall-back, and thus the decision upon which search engine is used can actually > be postponed until the user has typed some search-string. So it could > instantiate both the tracker and simple backends and if tracker has responded > until the user starts the search, that is used, otherwise the simple-fallback. I should add, I did call the Wait() API with a timeout of 1 second which should be fast enough to know if Tracker is available and not delayed due to internal updates on the DB.
(In reply to comment #13) > I just tested this. It doesn't crash if you close the dialog before a reply is > received. Additionally, the current tracker searchengine implementation use > tracker_sparql_connection_get() which does Wait() internally anyway. That was just speculation; one of the things that came to my mind that could happen, if you're spinning the mainloop in a place where no one reasonably would expect it to be spinning. Other things that possibly could go wrong is that the code is reentered for instance. As Bastien says, it's just a very bad idea. The right thing is to make it either fast enough to be blocking or to expose the "asychronicty" through the interface, i.e. add a signal that is emitted once the wait is over. Yes, that means that the current interface needs to change (which is maybe not an option, though it seems to be a completly internal interface). > Detecting tracker is available is not the problem, the Wait() makes sure that > the database is ready to be used. There are cases where we replay the journal > or need to apply any schema changes and that means the resources are not > available immediately. Ok. So how long can that take? Since the simple backend seems to do a full fs scan (which is darn slow), the user might actually want to wait more than 1 second for tracker to become ready? Feeding the search result back to the file-chooser is already done via signals, so delaying that for a couple of seconds in some corner-cases seems ok. Of course if we are talking minutes, then maybe the user might prefer to use the simple backend in the meanwhile. Anyhow, this is more of a (UI) design issue than a actual implementation problem imho. > AFAICS, it will only try the next one if the previous implementation returns > NULL. So that's not possible. Asynch behavior just can't be introduced into an API and implementation that was designed for synchronous usage. You'll need to change the "surroundings" as well (maybe even the UI design), or keep the thing synchronous. Or end up with something very fragile.
(In reply to comment #15) > (In reply to comment #13) > > I just tested this. It doesn't crash if you close the dialog before a reply is > > received. Additionally, the current tracker searchengine implementation use > > tracker_sparql_connection_get() which does Wait() internally anyway. > > That was just speculation; one of the things that came to my mind that could > happen, if you're spinning the mainloop in a place where no one reasonably > would expect it to be spinning. Other things that possibly could go wrong is > that the code is reentered for instance. As Bastien says, it's just a very bad > idea. The right thing is to make it either fast enough to be blocking or to > expose the "asychronicty" through the interface, i.e. add a signal that is > emitted once the wait is over. Yes, that means that the current interface needs > to change (which is maybe not an option, though it seems to be a completly > internal interface). I knew it was a bad idea, I really wanted suggestions and feedback initially ;) The interface doesn't provide the asynchronicity and the sync call is fast enough from testing here anyway. With the timeout at a reasonable level (1/2 second or 1 full second, that's also the backup there). I have a patch locally which works with that too. > > Detecting tracker is available is not the problem, the Wait() makes sure that > > the database is ready to be used. There are cases where we replay the journal > > or need to apply any schema changes and that means the resources are not > > available immediately. > > Ok. So how long can that take? Depends on the schema changes (if any) or if a journal replay is ongoing it can be quite long (with a lot of user data to replay after a potential corruption for example). We might be talking minutes anyway not seconds. > Since the simple backend seems to do a full fs > scan (which is darn slow), the user might actually want to wait more than 1 > second for tracker to become ready? It is quite unlikely to take > 1 second even when instantiating a process in normal conditions. > Anyhow, this is more of a (UI) design issue than a actual implementation > problem imho. > > > AFAICS, it will only try the next one if the previous implementation returns > > NULL. So that's not possible. > > Asynch behavior just can't be introduced into an API and implementation that > was designed for synchronous usage. I don't agree there, I don't think it's ideal but I see it happen from time to time. > You'll need to change the "surroundings" as > well (maybe even the UI design), or keep the thing synchronous. Or end up with > something very fragile. Well, I think that fits better indeed. I agree it can also lead to better stability.
I think for the initial port to 0.11/0.12, we should stick with the current 'block-up-to-a-second-then-give-up' approach. After 3.2, I want to drop the beagle implementation, and that is probably the right time to look at introducing some async get_engine api.
Martyn, any chance to wrap up the remaining review comments and get something ready soon ? I'm hoping to get this into a release this week.
Created attachment 196288 [details] [review] This patch uses the sync call with the timeout as a fallback (1 second) I also fix some of the issues Bastien brought up.
Review of attachment 196288 [details] [review]: ::: gtk/gtksearchenginetracker.c @@ +41,3 @@ +#define DBUS_INTERFACE_STATUS "org.freedesktop.Tracker1.Status" + +/* Time in second to wait for service before deciding it's not available */ I think Bastien was asking for a correction in the comment here, actually: 'Time in seconds' not 'Time in second' @@ +146,3 @@ + NULL, + NULL, + &error); Any reason not to simply use g_dbus_connection_call_sync() here ? Beats manual fiddling with the message, imo... @@ +315,3 @@ + get_query_results_cb, + res); + Same here: g_dbus_connection_call() should make this a bit more straightforward.
The following fix has been pushed: 7466f84 GtkSearchEngineTracker: port to tracker 0.12
Created attachment 196322 [details] [review] GtkSearchEngineTracker: port to tracker 0.12 We simply use the Tracker DBus api here, caching and direct access that come with libtracker-sparql are probably not needed here. Based on a patch by Martyn Russell.