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 662208 - failure to initialize a GInitable should be considered programming error
failure to initialize a GInitable should be considered programming error
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2011-10-19 15:21 UTC by Simon McVittie
Modified: 2011-11-23 13:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GInitable, GAsyncInitable: not initializing gives undefined behaviour (4.76 KB, patch)
2011-10-20 20:13 UTC, Simon McVittie
reviewed Details | Review

Description Simon McVittie 2011-10-19 15:21:41 UTC
+++ This bug was initially created as a clone of Bug #661689 +++

 <davidz> alexl: hey - re https://bugzilla.gnome.org/show_bug.cgi?id=661689
 I'm in favor of just changing the GInitable docs (e.g. replace "should
 return G_IO_ERROR_NOT_INITIALIZED" with "should g_warning(), g_critical") ...
 I just don't see how it can ever be valid to use a GInitable instance until
 it has been initialized (but maybe I'm not looking hard enough)... thoughts?
 <alexl> davidz: Can't think of any useful scenario either.
 <alexl> davidz: go for it
 <davidz> alexl: sure, I'll follow up in the bug

davidz then wrote:
> Thus, I think we should change the GInitable docs to suggest that
> use-before-initialization is a programming error in the general case ... and
> that programmers should e.g. g_return_if_fail(), g_critical() instead of
> returning G_IO_ERROR_NOT_INITIALIZED. Thoughts?

Dan Winship replied:
> agreed, and likewise for use-after-initialization-has-failed; the only thing
> you can do if g_initable_init() returns FALSE is destroy the object.
>
> The GInitable docs should probably also xref GAsyncInitable a bit more.
> (eg, if an object is both GInitable and GAsyncInitable, then you can
> call either g_initable_init() or g_async_initable_init_async() to
> initialize it).
>
> Also, presumably you are allowed to g_object_ref/unref() an initable before
> initializing it, which the docs currently technically forbid.

I'll write some patches for master for this, but I don't think it'd be appropriate to make this change in a stable branch.
Comment 1 Simon McVittie 2011-10-19 17:01:35 UTC
One argument against this is that (particularly if applied to properties and other generic GObject features) it makes both GInitable and GAsyncInitable into "anti-interfaces" - anything that interacts with GObjects generically is considered broken, unless it knows about them.

In particular, making an object implement an extra interface is normally a "safe" change that doesn't affect its ABI - everything that was valid before is still valid - but this isn't the case for the initable interfaces (although if you implement one, implementing the other is safe).

GInitiallyUnowned is a bit like this too - replacing a direct GObject subclass with a GInitiallyUnowned subclass can turn correct code into incorrect code.

(I agree that raising a GError makes very little sense, though.)
Comment 2 Dan Winship 2011-10-19 17:29:05 UTC
(In reply to comment #1)
> In particular, making an object implement an extra interface is normally a
> "safe" change that doesn't affect its ABI - everything that was valid before is
> still valid - but this isn't the case for the initable interfaces (although if
> you implement one, implementing the other is safe).

That's always been true though. If an object is not initable, you're obviously not going to have code checking if its methods return G_IO_ERROR_NOT_INITIALIZED, so if someone makes it initable later, you'll lose under either the current rules or the propsed new rules.
Comment 3 David Zeuthen (not reading bugmail) 2011-10-19 17:47:05 UTC
(In reply to comment #1)
> One argument against this is that (particularly if applied to properties and
> other generic GObject features) it makes both GInitable and GAsyncInitable into
> "anti-interfaces" - anything that interacts with GObjects generically is
> considered broken, unless it knows about them.
> 
> In particular, making an object implement an extra interface is normally a
> "safe" change that doesn't affect its ABI - everything that was valid before is
> still valid - but this isn't the case for the initable interfaces (although if
> you implement one, implementing the other is safe).

No one says you MUST NOT implement your type implementing GInitable in a way so using an instance of it before initialization is not considered a programming error.. IOW, it is valid to implement a type FooBar implementing GInitable and have one of its methods fail with G_IO_ERROR_NOT_INITIALIZED (rather than g_critical() etc) if it hasn't been successfully initialized.

I think all we're saying is that we RECOMMEND that people implement their types so it is considered a programming error if an instance of the type is used before it has been successfully initialized. And if we say this, we should probably stress that an application MAY NOT consider it a programming error if an instance is used before being successfully initialized.

I think the easiest thing is probably to say nothing at all in the GInitable docs... instead we'd mention in each GInitable type (such as GDBusConnection) that it is considered a programming error if an instance is used before being successfully initialized.

(and sorry for the shouting - I'm only shouting in the RFC 2119 sense :-) )
Comment 4 Simon McVittie 2011-10-19 17:59:35 UTC
(In reply to comment #3)
> And if we say this, we should
> probably stress that an application MAY NOT consider it a programming error if
> an instance is used before being successfully initialized.
...
> (and sorry for the shouting - I'm only shouting in the RFC 2119 sense :-) )

The reason that RFC 2119 doesn't give "MAY NOT" a meaning is that it's ambiguous :-) but I think what you mean is: an object implementation MAY consider method calls on an uninitialized instance to be either valid or invalid.

> I think the easiest thing is probably to say nothing at all in the GInitable
> docs... instead we'd mention in each GInitable type (such as GDBusConnection)
> that it is considered a programming error if an instance is used before being
> successfully initialized.

Thanks, I think this approach is better than redefining what GInitable means. In particular, GDBusConnection would mostly crash anyway - so I think it's OK to critical all over the place there - but I don't think it'd be a good idea to change all GSocket's graceful GErrors into criticals. Patch on the way...
Comment 5 Dan Winship 2011-10-19 18:17:52 UTC
(In reply to comment #3)
> No one says you MUST NOT implement your type implementing GInitable in a way so
> using an instance of it before initialization is not considered a programming
> error..

I do! I do! :)

Language bindings will call g_initable_init() for you automatically at construction time, so if there's some legitimate use case where a caller wouldn't want to initialize your type right away, then you shouldn't be using GInitable. You should just have your own initialize/prepare/connect/whatever method, and document how it should be used.


The original idea behind GInitable was "g_object_new() that can return a GError". No one should ever see an initable object that has been constructed but not yet inited, unless the object hands out a reference to itself from within constructed() or init().
Comment 6 Simon McVittie 2011-10-19 18:27:32 UTC
(In reply to comment #5)
> No one should ever see an initable object that has been constructed
> but not yet inited, unless the object hands out a reference to itself from
> within constructed() or init().

... or unless you call g_object_new() on it, which is how language bindings always used to make GObjects (if I understand correctly).

(The crash dumps that prompted this extended yak-shaving excursion indicated that GDBusConnection might be leaking one of its internal uninitialized refs out to user code... or the devices where the crashes happened might have had bad RAM, or there might have been heap corruption. We just don't know.)
Comment 7 David Zeuthen (not reading bugmail) 2011-10-19 18:31:32 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > No one says you MUST NOT implement your type implementing GInitable in a way so
> > using an instance of it before initialization is not considered a programming
> > error..
> 
> I do! I do! :)
> 
> Language bindings will call g_initable_init() for you automatically at
> construction time, so if there's some legitimate use case where a caller
> wouldn't want to initialize your type right away, then you shouldn't be using
> GInitable. You should just have your own initialize/prepare/connect/whatever
> method, and document how it should be used.
> 
> 
> The original idea behind GInitable was "g_object_new() that can return a
> GError". No one should ever see an initable object that has been constructed
> but not yet inited, unless the object hands out a reference to itself from
> within constructed() or init().

I agree with all that. But consider someone retrofitting a type FooBar to use GInitable - maybe it's nice to have old C-based g_object_new()-based code working while transitioning (maybe it's a huge distributed code-base)... in particular, the exsiting initialize/prepare/connect/whatever method could simply be calling g_initable_init().
Comment 8 Simon McVittie 2011-10-20 20:13:27 UTC
Created attachment 199583 [details] [review]
GInitable, GAsyncInitable: not initializing gives  undefined behaviour

This is the ISO C sense of undefined behaviour, in which
works-by-coincidence, critical warning, abort, demons-fly-out-of-your-nose
are all valid implementations.

---

This is the strictest version (i.e. what Dan said), but also the version that most directly contradicts the previous documentation. I'm open to suggestions.
Comment 9 Alexander Larsson 2011-10-21 06:45:00 UTC
Can it start NetHack? (http://www.feross.org/gcc-ownage/)
Comment 10 Simon McVittie 2011-10-21 10:07:04 UTC
(In reply to comment #9)
> Can it start NetHack? (http://www.feross.org/gcc-ownage/)

I'm now very tempted to add that somewhere. system() is portable, right? :-)
Comment 11 David Zeuthen (not reading bugmail) 2011-10-21 13:58:15 UTC
Review of attachment 199583 [details] [review]:

Looks good to me. Thanks for doing this.
Comment 12 Simon McVittie 2011-10-21 15:17:20 UTC
I'll leave this for a couple of days to give others a chance to veto/comment/approve, since it's pure documentation and doesn't really block anything (particularly now that GDBusConnection documents that you really do have to initialize first).
Comment 13 Simon McVittie 2011-11-23 13:03:37 UTC
(In reply to comment #12)
> I'll leave this for a couple of days to give others a chance to
> veto/comment/approve

Nobody did, so I merged it to master for 2.31.3. Commit 0104c62.