Skip to content

Fix memorty leak in rocksdb_wal_iter_get_batch function#5515

Closed
JimChengLin wants to merge 2 commits intofacebook:masterfrom
JimChengLin:master
Closed

Fix memorty leak in rocksdb_wal_iter_get_batch function#5515
JimChengLin wants to merge 2 commits intofacebook:masterfrom
JimChengLin:master

Conversation

@JimChengLin
Copy link
Contributor

wal_batch.writeBatchPtr.release() gives up the ownership of the original WriteBatch, but there is no new owner, which causes memory leak.

The patch is simple. Removing release() prevent ownership change. std::move is for speed.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @JimChengLin for the fix

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@riversand963 merged this pull request in cd25203.

merryChris pushed a commit to merryChris/rocksdb that referenced this pull request Nov 18, 2019
)

Summary:
`wal_batch.writeBatchPtr.release()` gives up the ownership of the original `WriteBatch`, but there is no new owner, which causes memory leak.

The patch is simple. Removing `release()` prevent ownership change. `std::move` is for speed.
Pull Request resolved: facebook#5515

Differential Revision: D16264281

Pulled By: riversand963

fbshipit-source-id: 51c556b7a1c977325c3aa24acb636303847151fa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants