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 646957 - GIO chained calls don't work with a thread default context
GIO chained calls don't work with a thread default context
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal major
: ---
Assigned To: gtkdev
gtkdev
: 646960 (view as bug list)
Depends on:
Blocks: 646960
 
 
Reported: 2011-04-06 21:20 UTC by Olivier Crête
Modified: 2011-09-26 16:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Simple test program (1.05 KB, text/plain)
2011-04-06 21:20 UTC, Olivier Crête
  Details
Simple test program (1.05 KB, text/plain)
2011-04-06 22:03 UTC, Olivier Crête
  Details
test program (for real this time) (1.29 KB, text/plain)
2011-04-06 22:04 UTC, Olivier Crête
  Details
GSimpleAsyncResult: push thread context around callback (1.66 KB, patch)
2011-04-20 15:50 UTC, Dan Winship
committed Details | Review

Description Olivier Crête 2011-04-06 21:20:19 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)
Comment 1 Olivier Crête 2011-04-06 22:03:22 UTC
Created attachment 185367 [details]
Simple test program

Oops, attached the wrong example the first time
Comment 2 Olivier Crête 2011-04-06 22:04:07 UTC
Created attachment 185368 [details]
test program (for real this time)
Comment 3 Dan Winship 2011-04-07 12:49:57 UTC
(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.
Comment 4 Olivier Crête 2011-04-07 14:37:58 UTC
(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.
Comment 5 Dan Winship 2011-04-07 14:48:13 UTC
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.
Comment 6 Olivier Crête 2011-04-07 15:12:51 UTC
Yes, I'm saying push/pop should be called by g_main_loop_run(), not that the app should do it.
Comment 7 Dan Winship 2011-04-07 16:21:27 UTC
uh, right, sorry, i was confusing myself
Comment 8 David Zeuthen (not reading bugmail) 2011-04-14 14:26:42 UTC
I think option 3. is the only safe bet here.
Comment 9 Dan Winship 2011-04-20 15:50:08 UTC
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.
Comment 10 Dan Winship 2011-04-20 15:52:04 UTC
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)
Comment 11 David Zeuthen (not reading bugmail) 2011-04-20 17:42:20 UTC
(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.
Comment 12 Dan Winship 2011-04-26 15:31:45 UTC
*** Bug 646960 has been marked as a duplicate of this bug. ***
Comment 13 Dan Winship 2011-04-26 15:33:25 UTC
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
Comment 14 Nicolas Dufresne (ndufresne) 2011-04-26 16:04:59 UTC
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.
Comment 15 Jens Georg 2011-09-24 06:56:50 UTC
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."
Comment 16 Colin Walters 2011-09-24 13:18:36 UTC
(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?
Comment 17 Nicolas Dufresne (ndufresne) 2011-09-24 20:00:53 UTC
(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.
Comment 18 Dan Winship 2011-09-24 20:06:02 UTC
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.
Comment 19 Jens Georg 2011-09-25 07:07:19 UTC
(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.
Comment 20 Dan Winship 2011-09-26 16:45:40 UTC
I've updated the docs