Created attachment 286975 [details] [review]
gcancellable: Clarify that GSources hold references to GCancellables

Clarify in the documentation that a GSource created with
g_cancellable_source_new() must be explicitly removed from its
GMainContext before the GCancellable can be finalised.

This could be a common way of leaking GCancellables.

Comment on attachment 286975 [details] [review]
gcancellable: Clarify that GSources hold references to GCancellables

>+ * The new #GSource will hold a reference to the #GCancellable, so will not be
>+ * automatically destroyed when your last reference to the #GCancellable is
>+ * dropped, and instead must be explicitly removed from its #GMainContext.

I'm fine with "The new #GSource will hold a reference to the #GCancellable", but the rest of the text seems to imply that this behavior is unexpected, which I don't think it is. (Are there any GSources that *don't* behave this way?)

(In reply to comment #2)
> (From update of attachment 286975 [details] [review])
> >+ * The new #GSource will hold a reference to the #GCancellable, so will not be
> >+ * automatically destroyed when your last reference to the #GCancellable is
> >+ * dropped, and instead must be explicitly removed from its #GMainContext.
> 
> I'm fine with "The new #GSource will hold a reference to the #GCancellable",
> but the rest of the text seems to imply that this behavior is unexpected, which
> I don't think it is. (Are there any GSources that *don't* behave this way?)

Hmm, I guess my thought when I first started using GCancellableSource* was that it would act as a weak notifier for the GCancellable so, yes, I assumed it would automatically destroy itself when the GCancellable went away. None of the other GSources document their reference behaviour either, so (unless I’m missing some documentation somewhere) there is no precedent there.

And the behaviour doesn’t necessarily become clear as soon as you start using GCancellable either, since GCancellables don’t generally get cancelled often.

*I’m just cleaning up and checking some code from that period, which is how I came across this docs omission.

Ping?

As I said, I'm find pointing out that the source holds a ref on the cancellable, but I don't think the docs should imply that this is unexpected.

Created attachment 289392 [details] [review]
gcancellable: Clarify that GSources hold references to GCancellables

Clarify in the documentation that a GSource created with
g_cancellable_source_new() must be explicitly removed from its
GMainContext before the GCancellable can be finalised.

This could be a common way of leaking GCancellables.

Created attachment 289393 [details] [review]
gmain: Document convention of strong refs in GSources

By convention, any GSource which holds a pointer to another GObject
always holds a strong reference. Document that.

tbh, I don't think any of this needs pointing out. this is just how reference counting works. Adding excessive detail like this to the docs in places only serves to muddle the picture and saw doubts in the minds of the reader: 'wait, does it work this way only if the docs point it out ?!'. No, it doesn't. It works this way everywhere. And that is why it doesn't need random documentation additions.

Review of attachment 289392 [details] [review]:

This one is fine.

Review of attachment 289393 [details] [review]:

This is probably too much indeed.

(In reply to comment #8)
> tbh, I don't think any of this needs pointing out. this is just how reference
> counting works. Adding excessive detail like this to the docs in places only
> serves to muddle the picture and saw doubts in the minds of the reader: 'wait,
> does it work this way only if the docs point it out ?!'. No, it doesn't. It
> works this way everywhere. And that is why it doesn't need random documentation
> additions.

Sure, normal reference counting doesn’t need pointing out. The problem here is that the relationship between GCancellable and GCancellableSource is unclear, and hence the strength of the refs is unclear.

GCancellableSource is effectively a notifier for GCancellable, and so I think it’s quite reasonable for someone to expect it to be destroyed when the GCancellable it’s watching is destroyed. So it clarifies the picture to make it explicit that it’s not the case.

Sorted.

Attachment 289392 [details] pushed as 203fe3b - gcancellable: Clarify that GSources hold references to GCancellables