fix(core): Memory leak bugs in CheckPoint::drop impl
#1997
Conversation
Fix memory leak bug in `CheckPoint::drop` by using `Arc::into_inner` if it is available (>= 1.70). Fix `CPInner::drop` logic so that if `CPInner::block` becomes generic and is of a type that required `drop`, it does not leak memory. Add tests for memory leak + stack overflow when dropping `CheckPoint`.
evanlinjin
force-pushed
the
fix/checkpoint-drop-memory-leak
branch
2 times, most recently
from
f7632fb to
4401886
Compare
| // Don't call `drop` on `CPInner` since that risks it becoming recursive. | ||
| core::mem::forget(node); |
There was a problem hiding this comment.
Rationale for removal:
core::mem::forget is dangerous if CPInner::block is generic since the type might require a call to drop. This is not possible now, however it's best to remove it now to avoid forgetting about it later on.
The initial motivation for calling forget is to ensure no recursion happens. However, the recursive depth would always only be 1 since prev is None. Thus, there is no risk of stack overflow (proven by the checkpoint_drop_is_not_recursive test that is added).
| if version_check::is_min_version("1.70.0").unwrap_or(false) { | ||
| println!("cargo:rustc-cfg=has_arc_into_inner"); | ||
| } | ||
| } |
There was a problem hiding this comment.
IMO this is slightly overkill and we can wait until MSRV is at least 1.70.0 to use Arc::into_inner.
There was a problem hiding this comment.
Memory leaks aren't fun though.
FWIW, bumping MSRV to 1.75 would be fine by us, especially if it allows you guys to avoid these kinds of complications (also cf. #1750) |
Description
CheckPoint::dropby usingArc::into_innerif it is available (>= 1.70).CPInner::droplogic so that ifCPInner::blockbecomes generic and is of a type that requireddrop, it does not leak memory.CheckPoint.An alternative fix is to bump MSRV to >= 1.70 so we don't need to do conditional compilation based on version.
Changelog notice
Checklists
All Submissions:
Bugfixes: