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

[Pieter Hintjens]

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.

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