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

Pieter Hintjens ph at imatix.com
Thu Sep 4 20:55:10 CEST 2014


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



More information about the zeromq-dev mailing list