Created attachment 174768 [details] [review]
Add vfuncs to GObjectClass to notify when a signal is connected/disconnected

It adds two new vfuncs to GObjectClass that are called from gsignal when a new signal is connected/disconnected. I'm not sure whether it would be better to include an empty implementation in gobject.c so that we don't need to check if they are NULL before using them.

Signals are not GObject-specific: in theory every pointer can have a signal installed.

also, AFAIR, with the current implementation, notification of signal connect/disconnect is inherently racy -- but I can't find a quote on that.

(In reply to comment #2)
> Signals are not GObject-specific: in theory every pointer can have a signal
> installed.

Yes, that's why I check whether it's an object or not before trying to get the GObjectClass pointer, this is supposed to be a feature for GObjects, it doesn't affect any other pointer using signals. 

> also, AFAIR, with the current implementation, notification of signal
> connect/disconnect is inherently racy -- but I can't find a quote on that.

I don't know, notifications always happen with the signal lock held.

At least for the WebKit bindings it would be useful to know how many handlers are present each time the notification is fired, that way when we reach 0 we can disconnect our stuff on the WebKit side. Perhaps both methods could be combined into just one with an integer parameter carrying the amount of handlers present. Otherwise we'll have to query glib for this data each time we are notified.

(In reply to comment #2)
> Signals are not GObject-specific: in theory every pointer can have a signal
> installed.
> 

We discussed this before sending the patch but:

a) There does not seem to be a reasonable way to implement this for every type.
b) I suspect implementing it in GObject covers more than 99.9% of all the possible use cases.

a), if true, is unfortunate, but it seems sad to not do this given b), how useful it would be for us and how simple the patch is.

(In reply to comment #2)
> also, AFAIR, with the current implementation, notification of signal
> connect/disconnect is inherently racy -- but I can't find a quote on that.

(In reply to comment #3)
> I don't know, notifications always happen with the signal lock held.

That's probably wrong. GObject generally doesn't call out to user code while holding global locks. Eg, the existing code always SIGNAL_UNLOCK()s before calling signal handlers or freeing objects that have destroy notifies.

But indeed, if you do that here, it will create race conditions, since once you SIGNAL_UNLOCK(), it's possible for other handlers to be added/removed, or for the signal to be emitted, before you call the notifier.

(In reply to comment #6)
> That's probably wrong. GObject generally doesn't call out to user code while
> holding global locks. Eg, the existing code always SIGNAL_UNLOCK()s before
> calling signal handlers or freeing objects that have destroy notifies.
> 
> But indeed, if you do that here, it will create race conditions, since once you
> SIGNAL_UNLOCK(), it's possible for other handlers to be added/removed, or for
> the signal to be emitted, before you call the notifier.

hmm, fair enough

(In reply to comment #4)
> At least for the WebKit bindings it would be useful to know how many handlers
> are present each time the notification is fired, that way when we reach 0 we
> can disconnect our stuff on the WebKit side. Perhaps both methods could be
> combined into just one with an integer parameter carrying the amount of
> handlers present.

Yeah, also, spare class struct slots are a non-renewable resource and it would be nice to only use 1 rather than 2. Maybe:

    void       (*signal_has_handlers)     (GObject        *object,
                                           const gchar    *detailed_signal,
                                           gboolean        has_handlers);

(Also, I think the un-detailed signal name would be more useful than the detailed name here?)

(In reply to comment #6)
> if you [unlock before notifying], it will create race conditions, since once you
> SIGNAL_UNLOCK(), it's possible for other handlers to be added/removed, or for
> the signal to be emitted, before you call the notifier.

Actually, this doesn't matter; in the connect-vs-emit case it's possible that you might get:

    Thread 1                                Thread 2

    g_signal_connect()
      {
        SIGNAL_LOCK()
        handler_insert()
        SIGNAL_UNLOCK()
                                            g_signal_emit()
        notify_connection_if_needed()
      }

but in that case if there was no synchronization between the two threads you could have just as easily ended up with

    Thread 1                                Thread 2

                                            g_signal_emit()
    g_signal_connect()

So the race condition was already there, and the notifier does not make it worse.

Likewise, as long as the signal-connecting code always decides whether or not to emit a notification while it's still holding the lock, I think the only way there can be a race condition between adding/removing handlers is if you use g_signal_handlers_disconnect_matched() in a way that could cause a signal handler to be removed from within the call to g_signal_connect() that was adding it. And again, that's already racy, since that would mean you were causing g_signal_connect() to return an already-invalid signal id. So again, not our problem.


>+  gobject_class = G_OBJECT_GET_CLASS (instance);
>+  if (gobject_class->signal_connected)
>+    gobject_class->signal_connected (G_OBJECT (instance), detailed_signal);

I feel like it would be cleaner to have this be in a GObject method, rather than having gsignal poking around in GObjectClass's innards.

Bug #380581 is an older version of this; let’s converge there.

Thanks for taking the time to report this.
This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find.

*** This bug has been marked as a duplicate of bug 380581 ***