[zeromq-dev] CZMQ: Error checking of z*_new() calls in other z*_new() functions

[Goswin von Brederlow]

On Fri, Sep 05, 2014 at 01:35:54PM +0200, Pieter Hintjens wrote:
> What do you think of this style, for instance (slight change to
> ziflist constructor):
> 
>         self->names = zlist_new ();
>         if (self->names)
>             self->addresses = zlist_new ();
>         if (self->addresses)
>             self->netmasks = zlist_new ();
>         if (self->netmasks)
>             self->broadcasts = zlist_new ();
>         if (self->broadcasts)
>             ziflist_reload (self);
>         else
>             ziflist_destroy (&self);
> 
> ...?
> 
> It's clearly correct by instant visual inspection, and trivial to maintain.

Neat trick. That wouldn't work for allocating self itself though.

How about this:

#define CHECK(x) if (!(x)) goto out

	CHECK (self = zmalloc (sizeof (self_t));
        CHECK (self->names = zlist_new ());
        CHECK (self->addresses = zlist_new ());
        CHECK (self->netmasks = zlist_new ());
        CHECK (self->broadcasts = zlist_new ());
        CHECK (ziflist_reload (self));
        ...
        return self;
    out:
        ziflist_destroy (&self);
        return self;

 
> On Thu, Sep 4, 2014 at 8:55 PM, Pieter Hintjens <ph at imatix.com> wrote:
> > I agree, and it was meant to be safe to call the destructor on an
> > unfinished object.
> >
> > I don't like the long lists of checks, they are a maintenance issue.
> >
> > It might be worth finding a better code style for constructors that
> > avoids this duplication and yet gives us clean all or nothing
> > construction. A goto might be neat. Or a series of chained if
> > statements. I'll experiment...
> >
> >
> >
> > On Thu, Sep 4, 2014 at 4:06 PM, Goswin von Brederlow <goswin-v-b at web.de> wrote:
> >> On Thu, Sep 04, 2014 at 11:51:27AM +0200, Pieter Hintjens wrote:
> >>> I'd prefer assertions in most places. The problem is that adding full
> >>> error handling on every allocation creates more complex code and
> >>> overall increases the risk of other errors.
> >>>
> >>> There are a few places where it's worth catching heap exhaustion, and
> >>> that is on message handling, ans you'll see this done in zmsg and
> >>> zframe for instance.
> >>>
> >>> However for the most part I don't think application developers are
> >>> competent to deal with out of memory situations correctly, and the
> >>> best strategy is to fail loudly and force the machine or architecture
> >>> to be fixed.
> >>>
> >>> So we can start by asserting inside constructors, and then fix those
> >>> constructors we want to, to return NULL on failures.
> >>
> >> What makes it complex is having to check every allocation and then
> >> error out if it failed. So you get tons of extra lines all doing the
> >> same (other than the check), possibly with more and more cleanup.
> >>
> >> One thing that I think keeps this managable is to not check every
> >> allocation by itself. Instead do all allocations in a block, then
> >> check if any of them has failed and only then start configuring or
> >> using them.
> >>
> >> Also the fact that destructors (and free) are save to call with a NULL
> >> pointer helps. So if any allocation fails you simply call the classes
> >> destructor and it will do any cleanup necessary. No need to check
> >> which resource where allocated and which not.
> >>
> >> Example:
> >>
> >> peer_t *
> >> peer_new (zframe_t *own_identity, zframe_t *peer_identity, proto_t *proto) {
> >>     peer_t *self = (peer_t *) zmalloc (sizeof (peer_t));
> >>     if (self) {
> >>         // allocate stuff
> >>         self->own_identity = zframe_strhex(own_identity);
> >>         if (peer_identity) {
> >>             self->peer_identity = zframe_strhex(peer_identity);
> >>         } else {
> >>             self->peer_identity = NULL;
> >>         }
> >>         self->proto = proto;
> >>         self->timeout_handle =
> >>             proto_timeout_add (self->proto, MIN_TIMEOUT, self);
> >>         self->out_msg = zlist_new ();
> >>         self->in_msg = zlist_new ();
> >>         self->requests = zhash_new ();
> >>
> >>         // did any allocation fail?
> >>         if (!self->own_identity || (peer_identity && !self->peer_identity) ||
> >>             !self->timeout_handle || !self->out_msg || !self->in_msg ||
> >>             !self->requests) {
> >>             peer_destroy (&self);
> >>             return NULL;
> >>         }
> >>
> >>         // safe to use allocated stuff (which this example doesn't :)
> >>         self->last_send_sequence = 0;
> >>         self->acked_send_sequence = 0;
> >>         self->max_recv_sequence = 0;
> >>         self->acked_recv_sequence = 0;
> >>         self->missed_beats = 0;
> >>         self->current_timeout = MIN_TIMEOUT;
> >>         self->peer_state = ACTIVE;
> >>         self->own_state = MESSAGE;
> >>         self->need_to_ack = false;
> >>     }
> >>     return self;
> >> }
> >>
> >> I would prefer checks to assert and asserts to crash & burn at some
> >> later time. I am able to deal with out-of-memory for the most part and
> >> an assert would prevent me from doing that. And if the code doesn't
> >> lend itself to handling out-of-memory I can always just assert().
> >> That's my choice then.
> >>
> >> MfG
> >>         Goswin
> >> _______________________________________________
> >> zeromq-dev mailing list
> >> zeromq-dev at lists.zeromq.org
> >> http://lists.zeromq.org/mailman/listinfo/zeromq-dev
> _______________________________________________
> zeromq-dev mailing list
> zeromq-dev at lists.zeromq.org
> http://lists.zeromq.org/mailman/listinfo/zeromq-dev