After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 561444 - Add async functions
Add async functions
Status: RESOLVED FIXED
Product: totem-pl-parser
Classification: Core
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: totem-pl-parser-maint
totem-pl-parser-maint
Depends on:
Blocks: 559628
 
 
Reported: 2008-11-18 22:47 UTC by Philip Withnall
Modified: 2009-06-17 13:57 UTC
See Also:
GNOME target: ---
GNOME version: 2.23/2.24


Attachments
Add asynchronous parsing functions to totem-pl-parser (17.77 KB, patch)
2008-11-18 22:58 UTC, Philip Withnall
rejected Details | Review
Add asynchronous parsing functions to totem-pl-parser (attempt 2) (23.12 KB, patch)
2009-06-05 13:55 UTC, Philip Withnall
committed Details | Review
Refactor recursive parsing to preserve options throughout the operation (44.90 KB, patch)
2009-06-16 20:35 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2008-11-18 22:47:39 UTC
+++ This bug was initially created as a clone of Bug #559628 +++

I created a m3u list with my favourite webradio stations. (See http://media.ubuntuusers.de/forum/attachments/1669857/webradio.m3u)
totem freeze on opening this file, but after waiting 5minutes everything works fine.

----

This bug can track adding the necessary async functionality to totem-pl-parser. Patch coming up for _parse and _parse_with_base.
Comment 1 Philip Withnall 2008-11-18 22:58:41 UTC
Created attachment 122991 [details] [review]
Add asynchronous parsing functions to totem-pl-parser

This has been tested a bit with both the totem-pl-parser test suite, and with a hackily-modified Totem, but probably wants a bit more testing before being committed.

Note that it adds new API: totem_pl_parser_parse_async() and totem_pl_parser_parse_with_base_async(), and breaks ABI by extending TotemPlParserClass.
Comment 2 Philip Withnall 2008-11-27 19:23:33 UTC
After reflection, I'm not so sure about using a signal (parsing-finished) to show that parsing's finished, rather than a callback function. Using a callback function would allow different userdata to be passed for each different parse operation, whereas with the signal approach, the signal handler has to be disconnected and reconnected for each new operation to change the userdata. Since the userdata will likely contain the MRL being parsed (or some other operation-specific identifier), this is less than optimal.
Comment 3 Steve Frécinaux 2008-12-09 08:36:05 UTC
What about reusing the GAsync* stuff from GIO ?
Comment 4 Christian Hergert 2008-12-09 22:37:47 UTC
You might also be interested in GTask, an Asynchronous toolkit library I've been writing for GObject.

Documentation:
  http://docs.dronelabs.com/gtask/
  http://docs.dronelabs.com/gtask/api

Sources:
  http://ftp.dronelabs.com/sources/gtask/0.1/gtask-0.1.2.tar.gz
  git://git.dronelabs.com/git/users/chris/gtask.git

-- Christian
Comment 5 Philip Withnall 2008-12-10 20:25:47 UTC
Steve: The GAsync* stuff doesn't seem to add anything useful to the API, and I think I'd only consider using it if the callback were to be a lot more complex (and in need of a custom object to pass it data, a la GAsyncResult).

Christian: Again, that seems a little overkill for this API.
Comment 6 Christian Hergert 2008-12-10 21:07:17 UTC
Philip, I'd certainly agree on the public API.  No need to add anything more than perhaps a new callback delegate.

At most, I was thinking it might make sense for use internally and allow to expose a callback delegate unique to totem which would be dispatched from a GTask callback.

Cheers
Comment 7 Bastien Nocera 2009-02-17 15:50:19 UTC
The code in totem_pl_parser_parse_with_base_async() is racy. It's possible that the thread already finished when we start adding more stuff to the queue. Either we should have a mutex, making it impossible for the thread to exit whilst we're adding more stuff, or you could keep a thread around all the time, and have it block with g_async_queue_timed_pop().

You should also probably lock and unlock the queue as necessary.

There also doesn't seem to be a way to cancel a parsing that was already started, which is a bit of a problem, and collides with the current sync behaviour of all the parsers.
Comment 8 Philip Withnall 2009-03-22 01:31:14 UTC
I'm going to rewrite this. After doing more async stuff with GAsyncResult elsewhere, I've decided it's probably a good idea to use that after all. That would also mean we can use GCancellable to allow the operation to be cancelled asynchronously.
Comment 9 Philip Withnall 2009-06-05 13:55:31 UTC
Created attachment 136023 [details] [review]
Add asynchronous parsing functions to totem-pl-parser (attempt 2)

This version uses GAsyncResult and GCancellable, and resolves all the issues with the first patch. In-progress parse operations can be cancelled from any thread using a GCancellable, and since each operation has its own thread, there are no race conditions on finishing the work queue.

As well as three new functions for async parsing (totem_pl_parser_parse_async, totem_pl_parser_parse_with_base_async and totem_pl_parser_parse_finish), it adds TOTEM_PL_PARSER_RESULT_CANCELLED, which is returned if a parse operation was cancelled before it finished.
Comment 10 Bastien Nocera 2009-06-05 14:33:31 UTC
The recurse level will be completely broken if we allow for multiple threads to access it...
Comment 11 Philip Withnall 2009-06-05 15:17:35 UTC
Any reason not to move that to being a parameter of totem_pl_parser_parse_internal? It would be cleaner.

It's not going to be as easy to make GAsyncResult stuff use our own thread and job queue.
Comment 12 Bastien Nocera 2009-06-09 20:31:01 UTC
(In reply to comment #11)
> Any reason not to move that to being a parameter of
> totem_pl_parser_parse_internal? It would be cleaner.

Sure, but that means it would need to be passed down all the functions, so that we can decrease it on the stack when recursing.

> It's not going to be as easy to make GAsyncResult stuff use our own thread and
> job queue.

Don't understand the problem here...
Comment 13 Philip Withnall 2009-06-09 22:25:58 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > Any reason not to move that to being a parameter of
> > totem_pl_parser_parse_internal? It would be cleaner.
> 
> Sure, but that means it would need to be passed down all the functions, so that
> we can decrease it on the stack when recursing.

Yes, but that doesn't look too big a job, from a quick grep. Maybe I've missed something…

> > It's not going to be as easy to make GAsyncResult stuff use our own thread and
> > job queue.
> 
> Don't understand the problem here...

Actually, thinking about it some more, the recurse level could get broken even if we only allowed async operations in one thread, since a sync operation could be started right after an async one, and they'd still contend the recursion level.
Comment 14 Bastien Nocera 2009-06-10 08:56:49 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Any reason not to move that to being a parameter of
> > > totem_pl_parser_parse_internal? It would be cleaner.
> > 
> > Sure, but that means it would need to be passed down all the functions, so that
> > we can decrease it on the stack when recursing.
> 
> Yes, but that doesn't look too big a job, from a quick grep. Maybe I've missed
> something…

It would update pretty much all the functions in totem-pl-parser that do parsing. That's quite a lot.

> > > It's not going to be as easy to make GAsyncResult stuff use our own thread and
> > > job queue.
> > 
> > Don't understand the problem here...
> 
> Actually, thinking about it some more, the recurse level could get broken even
> if we only allowed async operations in one thread, since a sync operation could
> be started right after an async one, and they'd still contend the recursion
> level.

Yes, which is why the recurse level needs to moved out of the TotemPlParser object itself, and be added in "parse" private data.
Comment 15 Philip Withnall 2009-06-16 20:35:48 UTC
Created attachment 136772 [details] [review]
Refactor recursive parsing to preserve options throughout the operation

This patch is to be applied on top of the previous one, and refactors the parsing stack to recursively use a TotemPlParseData struct, containing the recurse level, as well as some global-ish options like whether to use fallbacks or whether to recurse at all.

One upside to this approach is that TotemPlParserPrivate is now truly private to totem-pl-parser.c.
Comment 16 Bastien Nocera 2009-06-17 11:07:06 UTC
The problem is that all those:
+	data.fallback = fallback;
+	data.recurse = parser->priv->recurse;
+	data.force = parser->priv->force;
+	data.disable_unsafe = parser->priv->disable_unsafe;
aren't per-parse options, they're options for the object itself.

The only thing that's worth passing is the recurse_level. Other than that, it looks fine to commit.
Comment 17 Philip Withnall 2009-06-17 13:57:17 UTC
2009-06-17  Philip Withnall  <philip@tecnocode.co.uk>

	* docs/reference/totem-pl-parser-sections.txt:
	* plparse/plparser.symbols:
	* plparse/totem-pl-parser-builtins.c
	(totem_pl_parser_result_get_type):
	* plparse/totem-pl-parser-lines.c (totem_pl_parser_add_ram),
	(totem_pl_parser_add_m3u), (totem_pl_parser_add_ra):
	* plparse/totem-pl-parser-lines.h:
	* plparse/totem-pl-parser-media.c (totem_pl_parser_add_iso),
	(totem_pl_parser_add_cue), (totem_pl_parser_add_directory),
	(totem_pl_parser_add_block):
	* plparse/totem-pl-parser-media.h:
	* plparse/totem-pl-parser-misc.c (totem_pl_parser_add_gvp),
	(totem_pl_parser_add_desktop):
	* plparse/totem-pl-parser-misc.h:
	* plparse/totem-pl-parser-pla.c (totem_pl_parser_add_pla):
	* plparse/totem-pl-parser-pla.h:
	* plparse/totem-pl-parser-pls.c
	(totem_pl_parser_add_pls_with_contents), (totem_pl_parser_add_pls):
	* plparse/totem-pl-parser-pls.h:
	* plparse/totem-pl-parser-podcast.c (totem_pl_parser_add_rss),
	(totem_pl_parser_add_itpc), (totem_pl_parser_add_zune),
	(totem_pl_parser_add_atom), (totem_pl_parser_add_xml_feed),
	(totem_pl_parser_add_itms), (totem_pl_parser_add_opml):
	* plparse/totem-pl-parser-podcast.h:
	* plparse/totem-pl-parser-private.h:
	* plparse/totem-pl-parser-qt.c
	(totem_pl_parser_add_quicktime_rtsptext),
	(totem_pl_parser_add_quicktime_metalink),
	(totem_pl_parser_add_quicktime):
	* plparse/totem-pl-parser-qt.h:
	* plparse/totem-pl-parser-smil.c (totem_pl_parser_add_smil):
	* plparse/totem-pl-parser-smil.h:
	* plparse/totem-pl-parser-wm.c
	(totem_pl_parser_add_asf_reference_parser),
	(totem_pl_parser_add_asf_parser), (parse_asx_entry),
	(parse_asx_entryref), (parse_asx_entries),
	(totem_pl_parser_add_asx), (totem_pl_parser_add_asf):
	* plparse/totem-pl-parser-wm.h:
	* plparse/totem-pl-parser-xspf.c (totem_pl_parser_add_xspf):
	* plparse/totem-pl-parser-xspf.h:
	* plparse/totem-pl-parser.c (totem_pl_parser_class_init),
	(emit_playlist_ended_signal), (totem_pl_parser_playlist_end),
	(totem_pl_parser_is_debugging_enabled), (totem_pl_parser_init),
	(totem_pl_parser_finalize), (emit_entry_parsed_signal),
	(totem_pl_parser_add_uri_valist),
	(totem_pl_parser_scheme_is_ignored),
	(totem_pl_parser_mimetype_is_ignored),
	(totem_pl_parser_parse_internal), (parse_async_data_free),
	(parse_thread), (totem_pl_parser_parse_with_base_async),
	(totem_pl_parser_parse_with_base), (totem_pl_parser_parse_async),
	(totem_pl_parser_parse_finish),
	(totem_pl_parser_add_ignored_scheme),
	(totem_pl_parser_add_ignored_mimetype):
	* plparse/totem-pl-parser.h: Add asynchronous playlist parsing
	functions, totem_pl_parser_parse_async, *_parse_with_base_async and
	totem_pl_parser_parse_finish. This includes making TotemPlParser
	threadsafe, and ensuring it emits all its signals in the main thread.
	It also includes refactoring of recursive parsing so that global
	options are copied and preserved throughout the parse operation,
	and can't be changed mid-operation from another thread.
	(Closes: #561444)