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

Removes boxed closures to compile w/ latest nightly. #263

Closed
wants to merge 2 commits into from

Conversation

drbawb
Copy link
Contributor

@drbawb drbawb commented Jan 6, 2015

Boxed closures were removed in rust-lang/rust/pull/20578; their use is now a compile error.
Most of these changes are straightforward but I would like to draw special attention to commit 853c36e which fixes up src/sdl2/timer.rs

I discovered a safety violation: but I believe it was present in the previous revision with boxed closures as well. If the Timer is dropped before SDL fires the callback, and the remove_on_drop flag is not set:
then the C code is calling back into a stack-frame which just got freed.

Even putting the closure in a Box<Fn() -> uint> does not help here. The boxed closure would just be stored inside the Timer which was just dropped.

I believe the constructor should be split in two:

  1. A safe constructor which forces remove_on_drop to true.
  2. An unsafe constructor which allows the caller to set the remove_on_drop flags themselves.

remove_on_drop: false will only work in the case that the closure lives longer than the Timer<'a>.
Perhaps there is some other way we could express this w/ lifetime bounds, though?


Let me know your thoughts... but I believe this patch preserves the semantics of the previous revision.

drbawb added 2 commits January 6, 2015 09:38
This change is slightly more involved; the unboxed closure must be
stored as a trait object internally.

A pointer to that trait-object is then passed into the C callback as
`*const c_void`; dereferencing that gives us a callable closure again.
@bitonic
Copy link
Contributor

bitonic commented Jan 6, 2015

I've done the same but in a different way in #265. I think it's better to force the input to be a Box'ed unboxed closure, which doesn't require you to get a TraitObject (edit: I realise now that you might not need the TraitObject with your approach either -- not sure about it though). See https://github.com/bitonic/rust-sdl2/blob/912c644e370f62b62242ae71e51e81ad0d258f77/src/sdl2/timer.rs#L29 for my code, and rust-lang/rust@c7dd3c4 for a relevant commit.

I also think that stopping the timer when Timer is dropped is the only nice option. I don't think providing an unsafe interface is even worth it.

@drbawb
Copy link
Contributor Author

drbawb commented Jan 6, 2015

I had tried using a Box there. It still doesn't live long enough; the Boxed closure will be dropped as soon as Timer goes out of scope.

Both Box<Fn() -> uint> and &Fn() -> uint segfault on Mac OS 10.7.4 if the Timer is dropped before SDL fires the callback. I opted for avoiding the extra allocation. (EDIT: unless you specify remove on drop, of course.)

Mine should be equivalently as safe as storing the closure inside the Timer: because I have bounded the callback to 'a which means the closure is on a stack frame which lives [at least as long as] the lifetime of the Timer struct itself.

@bitonic
Copy link
Contributor

bitonic commented Jan 6, 2015

Wait, I agree that it's going to be trouble if the Timer is dropped before the callback is fired, which is why I stop the callback as soon as the Timer is dropped.

I see now that it's probably fine to not box the closure. I still don't see the need to transmute into a TraitObject though.

By the way, I wrote a little test, should be easy port to your code: https://github.com/bitonic/rust-sdl2/blob/912c644e370f62b62242ae71e51e81ad0d258f77/src/sdl2/timer.rs#L72

@drbawb drbawb closed this Jan 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants