Idle call race between TagReader and start of conversion

Reported by Michael Schwendt on 2013-02-10
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
SoundConverter
Critical
GautierPortet

Bug Description

In ui.py do_convert() I here observe a race between the TagReader and the real start of the the conversion. There 's an idle callback for tags_read() which adds the sound_file to the queue, but converter.start() is called prior than that and may skip files or process an empty queue even. If the idle callback adds a file to the queue later, that one will be available only for subsequent usage of the "Convert" button and leads to weird errors as two tasks in the queue want to work on the same file.

Michael Schwendt (mschwendt) wrote :

What's problematic here is that the tag reading can trigger the gstreamer plug-in search. If a missing plug-in is not found or not installed, the tag reader doesn't continue. Dunno yet what happens to the converter task queue in that case.

Michael Schwendt (mschwendt) wrote :

Reproducible with Fedora 19 development. Especially glib2 is much newer there:

  $ rpm -q glib2 gtk2 pygtk2
  glib2-2.35.7-1.fc19.x86_64
  gtk2-2.24.14-1.fc19.x86_64
  pygtk2-2.24.0-5.fc18.x86_64

For comparison, Fedora 18 is at glib2-2.34.2, gtk2-2.24.13, pygtk2-2.24.0.

One can observe that with Fedora 18, the tag reading is not race-free either. The last pending idle callbacks may add tasks to the converter queue after it has been started already. With regard to the task queue implementation, that's okay because it's an array "append", and the queue uses "pop" to retrieve waiting tasks. But if the tasks are added too late via idle calls, processing of the queue would have stopped already. That's easy to reproduce with Fedora 19 when converting a single audio file.

Other symptoms: Where with Fedora 18, any of the modal dialogs (such as "file exists - what to do?" or "missing plugin search") seem to block the main loop -- so if I close the first dialog, the next one is displayed -- with Fedora 19 only the first dialog is displayed and all other ones are ignored.

If this is not just temporary, it might be that something has changed to the gobject idle call behaviour and hence affects SoundConverter, where even the background tasks use idle calls.

summary: - Race between TagReader and start of conversion
+ Idle call race between TagReader and start of conversion
Changed in soundconverter:
assignee: nobody → GautierPortet (kassoulet)
importance: Undecided → Critical
Changed in soundconverter:
status: New → Confirmed
Michael Schwendt (mschwendt) wrote :

Surprise, surprise! :) With the latest fixes for gstreamer.py bug 1123410, it becomes possible to wait for all TagReader tasks to complete before starting conversion.

As I currently still use the forwarded ConverterQueueFoo exceptions, which work extremely well so far, what I do is similar to this in ui.py:

in read_tags():
        self.tag_reader_tasks.append(tagreader) # maintain list of TagReader tasks

in tags_read() callback:
        self.tag_reader_tasks.remove(tagreader) # maintain list of TagReader tasks
       # only add files where TagReader was successful
        if sound_file.tags_read: # or if not True, alternatively store an error to be found by do_convert() before converter.start
            self.converter.add(sound_file)

in do_convert():
        self.tag_reader_tasks = []
        self.converter.abort() # to start with empty queues

and at the end of the enumerate(files) loop, when all TagReader tasks have been started (but may not have completed yet):

            while len(self.tag_reader_tasks)>0 and len(self.converter.waiting_tasks) < total:
                gtk_sleep(1)

Of course, in that loop I check the forwarded exceptions, too. So far, this even seems to be a work-around for Fedora 19 development.

GautierPortet (kassoulet) wrote :

Yeah, all the current complex flow is stupid and prone to errors.
I don't remember why we are trying to read tags and convert at the same time, but this is way too complex.
I think this was used to remove tags from memory while we were converting, to decrease memory usage. But it's not worth it.

I wanted to fix all this when rewriting soundconverter 3.0 from scratch, but it seems not so hard to fix in fact.
Just read tags if needed, and them convert.

This is exactly what you are proposing BTW. I still have to figure what happens when we have a codec installation...

Michael Schwendt (mschwendt) wrote :

Codec installation is troublesome here, because

(1) gnome-packagekit has been broken for several releases of Fedora including current Fedora 18 ( https://bugzilla.redhat.com/756208 ). It crashes, which makes Soundconverter treat that as unsuccessful installation immediately. Only very rarely one can install a missing plugin that way.

(2) installation of the plugins is asynchronous on top of TagReader being based on BackgroundTask gobject idle callbacks. Currently, and especially with Fedora 19 development, I cannot be sure that TagReaders, which encounter a missing codec, are processed properly at all. One of the running tag readers results in the async dialog for codec installation, but the others don't receive the install_plugin_cb callback (and possibly not even a GST ASYNC_DONE message).

So, what's my work-around?

Based on all the fixes after 2.0.4, I've merged all patches into a single large one that was easier to compare with your 2.0.5 tree:
http://pkgs.fedoraproject.org/cgit/soundconverter.git/plain/soundconverter-2.0.4-fedora.patch

It still contains the forwarded exceptions work-around. And it waits for the tag readers to complete before starting the actual conversion. Because that is done, I forward another flag from inside the gstreamer Pipeline that tells the main UI whether a gstreamer plugin has been installed. If that happens during tag reading, I simply restart the entire do_convert step (= the tag reading!), and that may prompt for further missing plugins. Great! So far that even works with Fedora 19 development.

GautierPortet (kassoulet) wrote :

Wow, this is really sick and ugly!
And I'm afraid there is no better solution for now.

I'll try to restart only the jobs with a plugin install, to see if it works. But frankly, if plugin installation is broken, we really want to disable it for good.

GautierPortet (kassoulet) wrote :

Unfortunately, codec installation works as expected in Ubuntu. So removing it is a no-no. I'll wait for Fedora to fix it instead.

Michael Schwendt (mschwendt) wrote :

Sure, I could have disabled codec installation in Soundconverter, but that would only hide the breakage in gnome-packagekit.

An update to gnome-packagekit 3.6.2 here fixes codec installation for Fedora 18. And for Fedora 17, there's an update to 3.4.2, which is also supposed to fix that: https://admin.fedoraproject.org/updates/search/gnome-packagekit

GautierPortet (kassoulet) wrote :

This is getting ridiculous.
Now that I have a working fix, I wonder if tag reading is fixable after all.
I can't pause or cancel a tag reading, and it can't report errors. Aaarg!

Now I want to skip the tag reading, and rename temporary files after conversion,
but this leave even more problems in the wild...

GautierPortet (kassoulet) wrote :

Maybe a minimal fix will do the job. File renaming will disappear in version 3 anyway. Replaced by a SoundRenamer companion.

Michael Schwendt (mschwendt) wrote :

Hmm... cancelling tag reading is a matter of definition.

Here it works so far that the actual conversion won't be started anymore, but the loop that starts a tag reader for each file cannot be interrupted (because it iterates over a fixed list of files with no implementation that could break out of the loop). And if one of the tag readers has triggered async installation of a missing codec, the pending dialog will appear, too. The GStreamer exceptions are forwarded, however, so such errors would be handled.

Pausing the tag reading, oh well, that has not been implemented before. The existing code only tries to pause the converter queue.

GautierPortet (kassoulet) wrote :

My stress test involves the conversion of ~10000 files.

If I wait for all tags to be read, without cancelation possible... You better have to be sure before pressing "Convert" :)

Changed in soundconverter:
status: Confirmed → In Progress
milestone: none → 2.0.5
GautierPortet (kassoulet) wrote :

I've little free time these days, and the problem is complex, so it's going to be hard...

In fact, we are basically screwed here.
If we wait for the tag readers to finish, we have to implement pause/cancel, and it's very slow.
If we rename files after conversion, all overwrite/error stuff is broken. And what about network vfs?

And we want a fix ASAP, to release a new version.

The best and easiest solution may be to use one queue for both tags and conversion.
And be sure to add the following converter before "finishing" the tagreader.

The biggest challenge being that ConverterQueue must be replaced.

Another solution could be to add the tag reading in Converter or ConverterQueue, but it's not a great idea from a design point.

GautierPortet (kassoulet) wrote :

And I you expected, I've again changed my mind.

Using a join queue is way too complex , and there is no other gain.
Well, if we rename files after conversion, it's (theoretically) also faster.

So here it is, in a separate branch for now: https://github.com/kassoulet/soundconverter/commits/rename-after-convert

I have now to test if cancel/pause are still functioning.

Also, we don't ask to the user for overwrite/skip, we just generate new file names if needed.

GautierPortet (kassoulet) wrote :

Canceling is OK.

Pause has a race condition, if a task finishes while we are pausing, the next one will start anyway.

GautierPortet (kassoulet) wrote :

Tadaaaa. TagReader's no more!

https://github.com/kassoulet/soundconverter/compare/28d4ce6b89...75504cacd4

The next tests: Error handling, and Codecs installation...

Changed in soundconverter:
status: In Progress → Fix Committed
status: Fix Committed → In Progress
GautierPortet (kassoulet) wrote :

Codec installation still works as expected.

And message errors are OK now:
https://github.com/kassoulet/soundconverter/commit/f7209b8ff8d52f8b036c264f79bec750ccece32b

Changed in soundconverter:
status: In Progress → Fix Committed
Changed in soundconverter:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers