-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check for leaks #73
Check for leaks #73
Conversation
There is also more general refactoring done. |
pub fn strong_count(this: &Self) -> usize { | ||
this.inner.ref_cnt.load(SeqCst) | ||
pub fn strong_count(_this: &Self) -> usize { | ||
unimplemented!("no tests checking this? DELETED!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts + minor style nits.
} | ||
} | ||
|
||
struct NotifyWaker { | ||
notify: rt::Notify, | ||
pub(super) fn waker_vtable() -> &'static RawWakerVTable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit/TIOLI: It seems like this could just be a static
? Probably not a big deal.
} | ||
|
||
/// Track a raw allocation | ||
pub(crate) fn alloc(ptr: *mut u8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a thought (though definitely not a blocker for this PR): it would be cool if loom
's leak-checking code could (optionally?) capture the backtrace of the stack frame where the object was allocated from, so that the error reporting for leaks could include it?
} | ||
} | ||
|
||
struct NotifyWaker { | ||
notify: rt::Notify, | ||
pub(super) fn waker_vtable() -> &'static RawWakerVTable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit/TIOLI: can't this just be a static rather than a function?
} | ||
|
||
/// Track a raw deallocation | ||
pub(crate) fn dealloc(ptr: *mut u8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a thought (not a blocker for this PR): it would be really cool if loom
's leak-detection code could eventually capture the backtrace where an object was allocated from and display it to the user if that allocation was leaked. Could be worth doing in a follow-up...
|
||
impl State { | ||
pub(super) fn check_for_leaks(&self) { | ||
assert!(self.is_dropped, "object leaked"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an assert, it will panic immediately when the first leaked allocation is checked. It seems like we might (eventually) want to change this to return a bool
or something, so that Execution::check_for_leaks
can count the number of leaked Arc
s and raw allocations, and panic if either are non-zero, so that all leaks that occurred during an execution can be reported.
This would be especially nice if we also started tracking unique metadata (e.g. stack frames) per allocated object.
false | ||
}; | ||
|
||
if spurious { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit/TIOLI: could be simplified to
state.did_spur = state.did_spur || spurious;
|
||
match &mut store.entries[self.index] { | ||
Entry::Alloc(v) => v, | ||
_ => panic!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should this be unreachable!()
?
assert_eq!( | ||
1, | ||
std::sync::Arc::strong_count(&self.inner), | ||
"something odd is going on" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙃
A first pass at adding some leak checking to loom + other improvements.
Arc
is promoted to a full "object". This allows optimizing the behavior of the mocked arc with the loom scheduler. Now, using a mockedArc
requires less permutations as loom is able to "see" through it. Also, by promoting it to an object, at the end of each execution, all arcs are checked to see if they were correctly freed. In the event that they were not freed, the execution fails.The waker behind
block_on
is updated to use a mockedArc
. This allows catching leaking wakers.Mocked versions of
std::alloc::{alloc, dealloc}
are added. These do not impact the scheduler or permutations, but it allows tracking correct usage of allocation & deallocation within the context of an execution. If an allocation leaks, loom fail the execution.A
loom::alloc::Track<T>
type is added to allow tracking arbitrary values and ensure that the drop handlers are called. This functions in a similar way as allocation tracking.Finally,
Notify
is improved to permute on spurious wake ups. When waiting on a notify, loom will first try the execution without any spurious wake ups, then it will try the execution with a spurious wake up.futures::block_on
is updated to remove the double poll simulating spurious wake ups as doing so could hide bugs.