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

Make linenoise safe & various improvements to extra::rl #9096

Closed
wants to merge 5 commits into from

Conversation

huonw
Copy link
Member

@huonw huonw commented Sep 10, 2013

  • Wrap calls into linenoise in a mutex so that the functions don't have to be unsafe any more (fixes std::rl is unsafe #3921)
  • Stop leaking every line that linenoise reads.
  • Handle the situation of rl::complete(some_function); do spawn { rl::read(""); } which would crash (fail! that turned into an abort, possibly due to failing with the lock locked) when the user attempted to tab-complete anything.
  • Add a test for the various functions; it has to be run by hand to verify anything works, but it won't bitrot.

@huonw huonw mentioned this pull request Sep 10, 2013
unsafe {
// hasn't been called before: initialise the lock.
// XXX: probably incorrect wrt races.
rl_lock = Some(Mutex::new());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd guess this is wrong, since it's too late for me to actually work if this is correct/what is the correct solution.

@brson
Copy link
Contributor

brson commented Sep 10, 2013

Indeed this initialization is racy and it also leaks the Mutex, which contains allocations. Following this sort of approach requires fixing #9105 first, so that we can have allocation-free mutexes that can be statically initialized.

To fix this without doing #9105 first I'd suggest creating some C++ helper functions in rust_builtins.cpp, like rust_take_linenoise_lock / rust_drop_linenoise_lock.

@huonw
Copy link
Member Author

huonw commented Sep 11, 2013

Hm, this appears to be wrong (tab completion in rusti double-locks the mutex and makes it abort); deleted the r+, will investigate later.

@huonw
Copy link
Member Author

huonw commented Sep 11, 2013

Ok, deadlock resolved.

- Removes a layer of indirection in the storage of the completion
  callback.
- Handles user tab completion in a task in which `complete` hasn't been
  properly. Previously, if `complete` was called in one task, and `read`
  called in another, attempting to get completions would crash. This
  makes the completion handlers non-ambiguously task-local only.
- Fix a mismatch in return values between the Rust code and linenoise.
This test has to be run by a human, to check inputs etc. Fortunately, it
won't bitrot (syntactically, or type-check-ly; it might bitrot
semantically), as it is designed so that the test runner compiles it with
`--cfg robot_mode`, which is used to disable the actual running of code.
bors added a commit that referenced this pull request Sep 12, 2013
- Wrap calls into linenoise in a mutex so that the functions don't have to be `unsafe` any more (fixes #3921)
- Stop leaking every line that linenoise reads.
- Handle the situation of `rl::complete(some_function); do spawn { rl::read(""); }` which would crash (`fail!` that turned into an abort, possibly due to failing with the lock locked) when the user attempted to tab-complete anything.
- Add a test for the various functions; it has to be run by hand to verify anything works, but it won't bitrot.
@bors bors closed this Sep 12, 2013
@huonw huonw deleted the linenoise branch November 25, 2013 10:55
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
…arth

Fix `needless_borrow` 9095

fixes rust-lang#9095
changelog: Don't lint `needless_borrow` on method receivers when it would change which trait impl is called
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.

std::rl is unsafe
4 participants