Skip to content

Conversation

@shaitan
Copy link

@shaitan shaitan commented Apr 24, 2019

dnet_state_destroy() releases memory allocated for st->addrs and fills st's memory with 0xff.

library/net.c Outdated
dnet_sock_close(n, st->write_s);
err_out_free:
free(st);
dnet_state_destroy(st);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dnet_state_destroy calls pthread_mutex_lock(&st->trans_lock) as example. It'll be work correctly after destroy of mutex?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. I will add WIP label and rework it

@shaitan shaitan added the WIP Work in progress label Apr 24, 2019
dnet_state_put(st);
pthread_mutex_destroy(&st->send_lock);
pthread_mutex_destroy(&st->trans_lock);
dnet_state_put(st);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's undetermined what refcnt will left after dnet_state_move_to_dht call. In case of error, dnet_state_move_to_dht can put (-EEXIST) or not to put (other errorcode) the net state. So you can't know how many puts you must do at this line.

I think the good practic is to do like shared_ptr: we lock the resource in the component where we use this resource (in dnet_state_create), and don't do refcounting for another components. These another components must do clear refcounting by themself. For example: dnet_state_create must have scoped single refcnt-lock, and dnet_state_move_to_dht should inc (if success) or don't inc (if fail) refcnt. The same way in dnet_state_create: if success, we return net state and don't decrement refcnt; if fail, decrement refcnt and return NULL. The caller of dnet_state_create must keep the result object, or throw it away via put.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WIP Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants