Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Fix memory leaks on irohad shutdown #1968

Merged
merged 1 commit into from
Dec 21, 2018
Merged

Fix memory leaks on irohad shutdown #1968

merged 1 commit into from
Dec 21, 2018

Conversation

luckychess
Copy link
Contributor

Description of the Change

Pull request fixes couple of memory leaks on irohad shutdown by calling unsubscribe() in destructors of several classes.

Benefits

Clean output of leak sanitizer, Irohad destructor is called now.

Possible Drawbacks

None.

Signed-off-by: Konstantin Munichev <toobwn@gmail.com>
@l4l l4l added needs-review pr awaits review from maintainers bug bug/defect that was reproduced by maintainers labels Dec 20, 2018
@l4l
Copy link
Contributor

l4l commented Dec 20, 2018

How did you test, that the listed subscriptions leaks memory? I'm not really sure, but though that unsubscribe is called on such object destruction, fix me if I'm wrong

@luckychess
Copy link
Contributor Author

I built irohad target with leak sanitizer, than run and stop it. It clearly showed leaks without unsubscribe.

@neewy neewy added this to the rc2 milestone Dec 21, 2018
Copy link
Contributor

@lebdron lebdron left a comment

Choose a reason for hiding this comment

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

We decided to reorder fields in irohad in another pull request. The following destructor should be removed in the next pull request.
https://github.com/hyperledger/iroha/blob/03c5b1da2ccc8cb7e67909b2fb5b35f3fbfabb83/irohad/main/application.cpp#L516-L520

@luckychess luckychess merged commit f263e12 into dev Dec 21, 2018
@luckychess luckychess deleted the feature/irohad_dtor branch December 21, 2018 09:48
@l4l l4l removed the needs-review pr awaits review from maintainers label Dec 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug bug/defect that was reproduced by maintainers
Development

Successfully merging this pull request may close these issues.

None yet

5 participants