Fix memory leak in error::raise
#30
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When running [`ruby_memcheck`] against [`wasmtime-rb`]'s test suite, and Valgrind [detected a bunch of small leaks coming from `error::raise](https://github.com/bytecodealliance/wasmtime-rb/actions/runs/3457450066/jobs/5770886620). Turns out, `rb_raise` never frees the error message buffer so the memory for every raised `Error::Error` leaks. To fix this, `error:raise` now uses `rb_exc_new_str` which will allow the Ruby GC to free the `RString` error message. With this change, [Valgrind no longer detects the leak](https://github.com/bytecodealliance/wasmtime-rb/actions/runs/3457832824/jobs/5771647954). [`ruby_memcheck`]: https://github.com/Shopify/ruby_memcheck [`wasmtime-rb`]: https://github.com/bytecodealliance/wasmtime-rb
|
Oh wow, I went to such great lengths to avoid leaks when raising errors (i.e. the whole design of returning errors to raise), and then right there at the very last hurdle there's a leak just staring me in the face. I took a slightly different tack and (assuming no further oversights) fixed this in 8c870e3. |
|
Awesome, thanks @matsadler. Seems to resolve the issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When running
ruby_memcheckagainstwasmtime-rb's test suite, and Valgrind detected a bunch of small leaks coming from `error::raise.Turns out,
rb_raisenever frees the error message buffer so the memory for every raisedError::Errorleaks.To fix this,
error:raisenow usesrb_exc_new_strwhich will allow the Ruby GC to free theRStringerror message. With this change, Valgrind no longer detects the leak.