GNOME Bugzilla – Bug 660740
make GThread more standard
Last modified: 2011-10-14 14:23:00 UTC
We currently have a 'joinable' field to g_thread_new() If 'joinable' is FALSE, g_thread_new() returns a GThread* that you can't use for anything and is probably unsafe to even think about using (since the thread may already have exited by the time you go to look at it). Matthias discovered pthread_detach() which would allow us to make all threads joinable to start. If you wanted to ignore a thread, you could just detach() it immediately after creating it. That gives us two options: - g_thread_new() -> g_thread_join() or - g_thread_new() -> g_thread_detach() That led me to consider that since we have g_thread_new() maybe we should have g_thread_free() to accomplish the same effect as _detach() but that you would also need to call after _join(). So: - g_thread_new() -> g_thread_join() -> g_thread_free() or - g_thread_new() -> g_thread_free() That doesn't feel exactly right since, in the second case, you'd be calling g_thread_free() on a thread that still exists. Instead, we could make GThread a proper refcounted type, and say that g_thread_new() returns a reference to it (and you must call g_thread_unref() when you are done). - g_thread_new() -> g_thread_join() -> g_thread_unref() or - g_thread_new() -> g_thread_unref() Conceptually, the thread holds an implicit reference on itself for as long as it's running. I'm fond of this from the standpoint of how nicely it would bind and also how similar it is to the vast majority of our other APIs. It has a bit of a drawback in that people who are already used to our API (or the posix one) might expect that _join() does a full cleanup.
I think forcing an extra unref after join is a non-starter, compatibility-wise. If anything, you could think about adding a new function, eg g_thread_wait(), and then define join to be wait + unref.
but, going this way would give the GThread a more prominent, object-like posture, when in practise it is entirely useless as an object. There is _nothing_ you can do with a GThread...
the unref() after join would only be for threads created with g_thread_new()
(In reply to comment #2) > but, going this way would give the GThread a more prominent, object-like > posture, when in practise it is entirely useless as an object. There is > _nothing_ you can do with a GThread... (except join it :) Another possibility: have g_thread_new() always return a joinable thread, and have an alternate fire-and-forget method that just returns success/failure, not a GThread, which would be a wrapper around g_thread_pool_push(), using a max_threads=-1 exclusive=FALSE GThreadPool.
(In reply to comment #3) > the unref() after join would only be for threads created with g_thread_new() Given that we've just deprecated the create() methods, that does not really offer people a choice...
they would have two choices, as is always the case with deprecations: - continue to use the old interface or - adjust their code to the new interface
I basically think that a new interface that forces g_thread_free() on people is a nonstarter.
I've actually been thinking that it would be nice to have a GThreadSource -- that's another use for GThread. The nicest API I can imagine for GThreadSource would involve it taking a ref on the GThread...
to clarify: the GThreadSource would poll ready once the thread had exited
Created attachment 198283 [details] [review] Add g_thread_detach() Simplify g_thread_new() to always create joinable threads. At the same time, add g_thread_detach(), which allows to to detach a thread that started out as joinable. The implementation for win32 is currently lacking.
I think we need to remove the joinable parameter from new_full as well if we want to leave the door open to introducing refcounting later, since otherwise g_thread_new_full would give you a reference if you pass TRUE and not if you pass FALSE. Which would be bad api.
We both seem convinced that we're right, so I propose a compromise: ref()/unref() with join() causing an implicit unref(). I write the patch.
Created attachment 198469 [details] [review] Add g_thread_detach() Modified this so that it applies cleanly again.
okay. I discussed this with Matthias in Montréal and we came to an agreement about what to do. It's mostly done now (on master). Two open questions remain: - Behdad proposed that the thread func should take a GError** and that the get_results() type function should return it. This seems like a great idea at first blush, but then you start to wonder if we should also pass in a GCancellable*. Where to stop? I did try an implementation of this. We can do some nice things with it like: g_thread_abort_with_error (GError *error); and g_thread_abort (GQuark domain, gint code, const gchar *format, ...); - there is no wait() call yet, because I'm not sure how that should go some combination of: void wait(); gpointer wait(); and/or gpointer get_result(); /* implicitly waits */ gpointer get_result(); /* result if finished, else NULL */ gpointer get_result(); /* result if finished, else g_critical */
(In reply to comment #14) > - Behdad proposed that the thread func should take a GError** and that > the get_results() type function should return it. This seems like a > great idea at first blush, but then you start to wonder if we should > also pass in a GCancellable*. Where to stop? That might belong at a higher level of API... There are currently a bunch of ways to run something in another thread: - g_thread_new() - g_thread_pool_push() - g_io_scheduler_push_job() - g_simple_async_result_run_in_thread() I've been thinking about this lately because for some GTls stuff, we need to turn non-cancellable blocking function calls (PKCS#11 stuff) into cancellable sync/async gio methods. There's code to do that in GThreadedResolver, but it would need to be abstracted out, and then we'd have *another* run-something-in-a-thread API... So I was thinking, maybe it would be possible to do some merging and deprecating. GIOScheduler is really mostly just a GThreadPool now that the non-threaded case is gone, so it could pretty easily be deprecated. And I was fiddling around with making a "GTask" API that encapsulates the GIOScheduler and GThreadedResolver use cases, while also being an implementation of GAsyncResult, allowing it to replace GSimpleAsyncResult too (since GSimpleAsyncResult has gotten pretty seriously non-simple...)
I'm think I'm convinced that we shouldn't support GError** at this level. Can you open a new bug with your thoughts?
(In reply to comment #16) > Can you open a new bug with your thoughts? bug 661767
Thanks. I'm going to punt on the other issue -- we can do it after .0 is out. Closing this.