search

gchp / rustbox

Rust implementation of the termbox library
MIT License
469 stars 48 forks source link

Add thread safety around RustBox. #71

Closed Schoonology closed 6 years ago

Schoonology commented 7 years ago

The effects are twofold:

  1. Remove the !Send PhantomData element from RustBox, allowing a RustBox container to be shared between threads.
  2. Require mutable references in all places where the underlying termbox state will be changed. This requires all shared RustBox instances to be wrapped in an Arc<Mutex<>>.

Additionally, a "thread-safety" example was added.

Schoonology commented 7 years ago

I apologize for submitting a PR without first discussing the changes in an issue, and I know this seems like a duplicate of #44. The difference with this Pull Request is that it's far more wide-reaching, yet it does a better job of guaranteeing safety in the face of state changes going on within termbox.

I need this change for an application I'm building in an effort to render updates constantly while allowing user input. The other alternative I considered was splitting the master RustBox container into an input half and a rendering half, but the same thread safety work would be guaranteed to prevent multiple writes to the underlying termbox state.

jmacdonald commented 6 years ago

I think changing Rustbox's external API to be mutable is something we want to avoid. See this comment for an explanation as to why we should maintain Rustbox's interior mutability.

jmacdonald commented 6 years ago

@gchp I think we can safely close this, as it's addressed by #81.

gchp commented 6 years ago

Thanks @Schoonology for the work here. This is addressed in a different way in #81. Let me know if you have any other thoughts/issues! And thanks @jmacdonald for the patch.

Schoonology commented 6 years ago

@gchp My pleasure!