Skip to content

Conversation

@zizhong
Copy link
Member

@zizhong zizhong commented Aug 23, 2018

This is to fix the memory leak issue #4137 .

@zizhong zizhong self-assigned this Aug 23, 2018
@zizhong zizhong added this to the 9.0.0 milestone Aug 23, 2018
@zizhong
Copy link
Member Author

zizhong commented Aug 23, 2018

better to backport this if needed.

@zizhong
Copy link
Member Author

zizhong commented Sep 5, 2018

kindly ping if you got time. @bryancall @zwoop @SolidWallOfCode @shinrich @scw00 @oknet

shinrich
shinrich previously approved these changes Sep 6, 2018
Copy link
Member

@shinrich shinrich left a comment

Choose a reason for hiding this comment

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

I am not very familiar with the cppapi, but your logic does seem reasonable to track and free the memory associated with the promise.


#pragma once

#include <list>
Copy link
Member

Choose a reason for hiding this comment

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

Do you still need <list>?

Copy link
Member Author

Choose a reason for hiding this comment

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

should be removed

@zizhong
Copy link
Member Author

zizhong commented Sep 10, 2018

@shinrich @SolidWallOfCode list header is removed.

shinrich
shinrich previously approved these changes Sep 10, 2018
Copy link
Member

@shinrich shinrich left a comment

Choose a reason for hiding this comment

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

Still looks good to me.

@zizhong
Copy link
Member Author

zizhong commented Sep 12, 2018

Conflicts resolved.

@zizhong
Copy link
Member Author

zizhong commented Sep 12, 2018

@shinrich @SolidWallOfCode mind take another look? Thank you very much!

@zizhong zizhong merged commit 4442c62 into apache:master Sep 12, 2018
@bryancall bryancall modified the milestones: 9.0.0, 8.0.1 Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants