GNOME Bugzilla – Bug 572844
Helper for GCancellable::cancelled connect/disconnect
Last modified: 2009-05-15 08:48:35 UTC
There are a few possible races and complications wrt connecting and disconnecting from the "cancelled" signal in GCancellable that can be made a lot better with some helper functions. Issues: #1: There is a race condition between connecting to the "cancelled" signal and the cancellable going cancelled that means we can miss the signal being emitted. And it doesn't help checking is_cancelled() first, since there race is still there. However the GCancellable code can use the mutex to ensure this is ok. This can be worked around in user code by checking is_cancelled() after commiting and handling it there, however that risks racing with the signal being emitted inbetween, so that needs complicated code to protect against dual emission. #2: There is a race between signal emission and disconnecting the cancellable handler such that you can't guarantee that the handler is not executed after the handler is disconnected, and thus its not safe to free things the handler uses. In bug 553426 we added a lot of docs to describe how you can use a GClosureNotify to free the state, as this ensures that all handlers are fully run. However, this is rather messy and code would look much cleaner if we could use the disconnect as a barrier. This could also be handled with the addition of a helper function inside the GCancellable code by using the internal locks.
Created attachment 129325 [details] [review] Helpers for cancellable connect/disconnect
Created attachment 129328 [details] [review] Use the new API internally
I thought about doing something like this when I filed bug 553426, but I thought that having g_cancellable_disconnect() block would be a problem, because the "cancelled" signal handler might do something time-consuming, and so you wouldn't be able to safely use g_cancellable_disconnect() from the main thread, since it might block the UI. I guess one way to deal with that is to clarify that if you're worried about blocking, you don't *have to* use g_cancellable_disconnect(), but if you don't, then you have to use the data_destroy_func rather than cleaning up by hand. (Maybe g_cancellable_disconnect() should be explicitly something like "g_cancellable_disconnect_and_wait()" in that case to make it totally clear?) I guess actually, if the "cancelled" handler does something time-consuming, then you can't call g_cancellable_connect() from the UI thread either... is calling the cancellation handler from g_cancellable_connect() really the right thing to do for a "pre-cancellation"? Especially if there's a separate data_destroy_func, it seems like there shouldn't really be anything to do in the cancellation callback, since the cancellable op hasn't really started yet. So maybe the contract should just be that if g_cancellable_connect() returns 0, then the caller is expected to handle the cancellation by hand? I guess we'll see what makes sense once you start porting gvfs code to use it. > + * signal. Also handles the race condition that may happen > + * if the cancellable is cancellable right before connecting. "if the cancellable is cancelled" > + * Returns: The is of the signal handler or 0 if @cancellable has already s/is/id/ g_cancellable_reset() needs to check cancelled->cancel_running, and fail if it's set, since otherwise you could reset and re-cancel the cancellable from inside the signal handler, and it would just be a mess. g_cancellable_cancel() might make more sense now if you got rid of the "cancel" local variable and put everything into a single if? I can't think of any corresponding race conditions that affect g_cancellable_get_fd()/make_pollfd(), although we don't currently explain that you're not supposed to actually read() off the fd yourself when it polls readable, and we should.
Created attachment 129335 [details] [review] Helpers for cancellable connect/disconnect New version based dans feedback: Fixed doc typos. add comment saying that cancelled is not supposed to block. check in g_cancellable_reset that !cancelled_running. Added docs in g_cancellable_get_fd()/make_pollfd() saying you should not read from the fd.
Created attachment 129336 [details] [review] Use the new API in gvfs
We're in API freeze, so lets wait with this until post release.
Is this related to the race pointed out in bug 546181 ?
Matthias: No, that one is a race between cancellation and calling the user callback when finished. If you have a model where cancellation means that callback won't be called then there is a race where cancellation and finishing happens simultaneously in different threads. In gnome-vfs we have this problem, which results in possible leaks when we race like this. But due to the design of cancellation in GIO this problem doesn't exist.
Please commit these patches to master, thanks.
commited the glib patches, waiting with the gvfs one until we branch.
The patch that was committed says Since: 2.20 - this should be Since: 2.22 instead.
Commited the gvfs part.