Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix memory leak #3

Merged
merged 3 commits into from
Dec 23, 2021
Merged

fix memory leak #3

merged 3 commits into from
Dec 23, 2021

Conversation

yikenan
Copy link
Contributor

@yikenan yikenan commented Dec 22, 2021

to fix this bug:
oatpp/oatpp#445

Copy link
Member

@lganzzzo lganzzzo left a comment

Choose a reason for hiding this comment

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

Hey @yikenan ,

Thanks for the PR!
Looks great!

Please change OATPP_ASSERT to OATPP_LOGE and it's ready to merge!

@@ -63,6 +63,11 @@ DeflateEncoder::DeflateEncoder(v_buff_size bufferSize, bool gzip, v_int32 compre

}

DeflateEncoder::~DeflateEncoder() {
v_int32 res = deflateEnd(&m_zStream);
OATPP_ASSERT(res == Z_OK);
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the assert.
Let's just log the error instead:

OATPP_LOGE("[oatpp::zlib::DeflateEncoder::~DeflateEncoder()]", "Error. Failed call to 'deflateEnd()'.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. I have modified the code.
The same problem was found in the decoder and fixed.
Please review, thank you!

@yikenan
Copy link
Contributor Author

yikenan commented Dec 23, 2021

I ran the test program with Valgrind and found no memory leak now.

@lganzzzo lganzzzo merged commit e731806 into oatpp:master Dec 23, 2021
@lganzzzo
Copy link
Member

Merged!
Great work!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants