Skip to content
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

Memory leak from crossbeam_epoch::deferred::Deferred::new #464

Closed
jonhoo opened this issue Jan 29, 2020 · 12 comments
Closed

Memory leak from crossbeam_epoch::deferred::Deferred::new #464

jonhoo opened this issue Jan 29, 2020 · 12 comments

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Jan 29, 2020

I'm trying to run flurry (which uses crossbeam-epoch) through miri (after patching in #458), and it seems to be finding a number of memory leaks in

crossbeam_epoch::deferred::Deferred::new::call::<fn() {crossbeam_epoch::internal::no_op_func}>

Judging from the signature above, I don't think this is a bug in flurry, but I could be wrong (the PR is at jonhoo/flurry#37). Has there been an attempt at running the crossbeam-epoch test suite through miri following #458? You probably need to be on the very latest nightly since rust-lang/miri#1150 landed only very recently.

@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 29, 2020

Ah, interesting, this seems to be known from spacejam/sled#937 (comment). Not sure if we want to leave this open as an issue?

@ghost
Copy link

ghost commented Jan 29, 2020

I'm not in the loop but wonder: does miri support threads, or is there perhaps a plan to add support for it? That's the main reason why we're not using miri with crossbeam. Although, we could still use miri for single-threaded tests even though that doesn't give us complete test coverage.

jonhoo added a commit to jonhoo/flurry that referenced this issue Jan 29, 2020
These are probably spurious; see
crossbeam-rs/crossbeam#464
and
spacejam/sled#937 (comment)

And besides, we're now running both the leak sanitizer and the address
sanitizer.
@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 29, 2020

No, I don't think it does, which is probably a big part of the issue. Over in spacejam/sled#937 (comment), @divergentdave writes:

Presumably these are all hanging around still because Miri is single threaded, and nothing has explicitly run to clean them up yet.

So that may very well be the issue, I'm not sure. It's possible to turn off the miri leak checker (with -Zmiri-ignore-leaks), and then crossbeam-epoch seems to pass for me in flurry (with #458 that is)! 🎉 I'm also running the flurry test suite with both the leak and address sanitizers, and they don't report anything bad happening, so that makes me think that this is indeed an erroneous warning from miri. Are we confident enough to close this?

@ghost
Copy link

ghost commented Jan 29, 2020

I think we can close. :) Thanks for the detailed report! If you run into any issues again or if something fishy appears, feel free to reopen.

@ghost ghost closed this as completed Jan 29, 2020
@divergentdave
Copy link

After looking at crossbeam-epoch further, the lack of threads isn't the reason for the leak report, but rather, the closures from Deferred are ultimately owned by 'static variables. (It's a kind of intentional leak. The Deferred objects are in arrays, and they get replaced with placeholders during collection.)

Thanks for the pointer about -Zmiri-ignore-leaks, that'll be helpful!

@RalfJung
Copy link
Contributor

Yeah I would have been surprised if threads are the reason -- Miri will bail if you try to spawn a thread, so if there was a background thread started somewhere we'd notice (as opposed to silently not running that code).

After looking at crossbeam-epoch further, the lack of threads isn't the reason for the leak report, but rather, the closures from Deferred are ultimately owned by 'static variables.

Looks like you ran into rust-lang/miri#940 then.

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 7, 2020

Now that rust-lang/miri#940 has been closed, we should maybe consider enabling the miri leak check in CI again? /cc @jeehoonkang

@RalfJung
Copy link
Contributor

RalfJung commented Apr 7, 2020

Note that rust-lang/rust#70897 has to land before the fix will be available via rustup.

@jeehoonkang
Copy link
Contributor

@jonhoo sure, would you please send a PR after rust-lang/rust#70897 is merged?

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 10, 2020

@jeehoonkang that just landed. I'm a little strapped for time at the moment, but if you want to take a stab it should just be a matter of doing these steps:

  • curl -s https://rust-lang.github.io/rustup-components-history/x86_64-unknown-linux-gnu/miri to get the latest nightly that supports miri
  • rustup toolchain install <that nightly>
  • rustup component add miri
  • cargo miri setup
  • cargo miri test or cargo miri run the binary you want to run miri on.

@RalfJung
Copy link
Contributor

See rust-lang/miri#1318 for the problems we are still having with the leak checker.

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 23, 2020

Now that miri supports threads and has the miri_static_root function, it should be possible to meaningfully run crossbeam under miri again! Unfortunately I'm pretty swamped right now, but if someone wants to give it a shot, it shouldn't be too hard.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants