GNOME Bugzilla – Bug 646957
GIO chained calls don't work with a thread default context
Last modified: 2011-09-26 16:45:40 UTC
Created attachment 185363 [details] Simple test program If a GIO-based library uses chained calls, the callback from the first call no longer has the thread default main context from the original call, so any subsequent async calls use the wrong GMainContext. I see three solutions: 1. Go through every user of chained async calls and do "g_main_context_{push,pop}_thread_default(res->context);" around any chained call. 2. Call push/pop at the start/end of g_main_loop_run(), so that everything call in that main loop will come back there unless explicitly specified. 3. Call push/pop around the call to the callback in g_simple_async_result_complete(), although that doesn't help for other GAsyncResult implementations. Attached is a small example, compile with: gcc -o test2 test2.c $(pkg-config --libs --cflags gio-2.0)
Created attachment 185367 [details] Simple test program Oops, attached the wrong example the first time
Created attachment 185368 [details] test program (for real this time)
(In reply to comment #0) > 2. Call push/pop at the start/end of g_main_loop_run(), so that everything call > in that main loop will come back there unless explicitly specified. This seems reasonable. However, it doesn't completely solve the problem from an intermediate library's perspective. Eg, if an app calls soup_socket_connect_async() and passes it a GMainContext, libsoup needs to ensure that the entire g_socket_client_connect_async() chain runs under that GMainContext. But the GMainLoop would have been started by the app, and libsoup has no control over whether or not the app pushed the context. (And just saying that the app is required to have pushed the context would be an ABI break, because the same code would have worked fine without that in the pre-gio-based libsoup.) > 3. Call push/pop around the call to the callback in > g_simple_async_result_complete(), although that doesn't help for other > GAsyncResult implementations. I think it's assumed at this point that there will not be other GAsyncResult implementations. No one has found any real use for one yet. If this change doesn't break any of gio, libsoup or gstreamer's tests, then I think it's probably the fix we want.
(In reply to comment #3) > (In reply to comment #0) > > 2. Call push/pop at the start/end of g_main_loop_run(), so that everything call > > in that main loop will come back there unless explicitly specified. > > This seems reasonable. > > However, it doesn't completely solve the problem from an intermediate library's > perspective. Eg, if an app calls soup_socket_connect_async() and passes it a > GMainContext, libsoup needs to ensure that the entire > g_socket_client_connect_async() chain runs under that GMainContext. But the > GMainLoop would have been started by the app, and libsoup has no control over > whether or not the app pushed the context. (And just saying that the app is > required to have pushed the context would be an ABI break, because the same > code would have worked fine without that in the pre-gio-based libsoup.) I meant calling push/pop inside g_main_loop_run(), so that anything run by that loop has the loop's context as its thread default main context.
libsoup doesn't call g_main_loop_run() itself, and (for backward-compatibility reasons) it has to work even if its caller didn't call g_main_context_push_thread_default() before starting the loop that it's asking libsoup to run in.
Yes, I'm saying push/pop should be called by g_main_loop_run(), not that the app should do it.
uh, right, sorry, i was confusing myself
I think option 3. is the only safe bet here.
Created attachment 186367 [details] [review] GSimpleAsyncResult: push thread context around callback When an old pre-thread-default-context API that takes an explicit GMainContext wants to call a gio API, it must call g_main_context_push_thread_default() before, and g_main_context_pop_thread_default() after the gio call, so that the gio method will return its result to the desired GMainContext. But this fails for methods like g_socket_client_connect_async() that make a chain of multiple async calls, since the pushed/popped context will only affect the initial call. Fix this by having GSimpleAsyncResult itself push/pop the context around the callback invocation, so that if the callback queues another async request, it will stay in the same context as the original one.
This works for libsoup, with or without the current workaround for the lack of this feature (http://git.gnome.org/browse/libsoup/tree/libsoup/soup-address.c#n636)
(In reply to comment #10) > This works for libsoup, with or without the current workaround for the lack of > this feature > (http://git.gnome.org/browse/libsoup/tree/libsoup/soup-address.c#n636) Does the gdbus test suite pass with this patch? If so (and I assume it would pass), full ACK from me.
*** Bug 646960 has been marked as a duplicate of this bug. ***
libsoup passes, gdbus passes, wocky passes. I think it's safe to say it works. Attachment 186367 [details] pushed as df45856 - GSimpleAsyncResult: push thread context around callback
Seem good to me, was one of two the solutions we imagined. The other one was the same, but in the main loop, we thought it would fix it more widely, but didn't know if there was any consequences.
May I note that this patch breaks the documented behavior: http://developer.gnome.org/gio/2.29/GSimpleAsyncResult.html#g-simple-async-result-complete-in-idle "Completes an asynchronous function in an idle handler in the thread-default main loop of the thread that simple was initially created in."
(In reply to comment #15) > May I note that this patch breaks the documented behavior: > > http://developer.gnome.org/gio/2.29/GSimpleAsyncResult.html#g-simple-async-result-complete-in-idle > > "Completes an asynchronous function in an idle handler in the thread-default > main loop of the thread that simple was initially created in." Jens, does this actually break any existing code you know of? Or are you just suggesting the documentation should be updated?
(In reply to comment #15) > May I note that this patch breaks the documented behavior: > > http://developer.gnome.org/gio/2.29/GSimpleAsyncResult.html#g-simple-async-result-complete-in-idle > > "Completes an asynchronous function in an idle handler in the thread-default > main loop of the thread that simple was initially created in." No, it's the total reverse. When an SimpleAsync is created, the default context thread context was saved, but never pushed, resulting in callback to escape and be executed in the main thread (the thread where main was executed earlier). In most cases, this is correct, but in more complex programs, it's not. Never the less, I think this should have been done on Glib side, pushing the GSource context before calling back, this would have been more accurate and fix more of those situations, not only the case of GSimpleAsyncResult users. But I'm saying that over my hat, as I know that this thread context thing was added for GIO.
We put as much in glib as we could; if we had changed the behavior of standard GSources, it would have broken tons of existing code.
(In reply to comment #16) > (In reply to comment #15) > > May I note that this patch breaks the documented behavior: > > > > http://developer.gnome.org/gio/2.29/GSimpleAsyncResult.html#g-simple-async-result-complete-in-idle > > > > "Completes an asynchronous function in an idle handler in the thread-default > > main loop of the thread that simple was initially created in." > > Jens, does this actually break any existing code you know of? Or are you just > suggesting the documentation should be updated? Yes, break was a bit harsh maybe. I meant documentation needs fixing.
I've updated the docs