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

[Goswin von Brederlow]

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