Make linenoise safe & various improvements to extra::rl

src/libextra/rl.rs

@@ -8,13 +8,10 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-// FIXME #3921. This is unsafe because linenoise uses global mutable
-// state without mutexes.
-
 use std::c_str::ToCStr;
 use std::libc::{c_char, c_int};
-use std::local_data;
-use std::str;
+use std::{local_data, str, rt};
+use std::unstable::finally::Finally;
 
 #[cfg(stage0)]
 pub mod rustrt {
@@ -28,6 +25,9 @@ pub mod rustrt {
         fn linenoiseHistoryLoad(file: *c_char) -> c_int;
         fn linenoiseSetCompletionCallback(callback: *u8);
         fn linenoiseAddCompletion(completions: *(), line: *c_char);
+
+        fn rust_take_linenoise_lock();
+        fn rust_drop_linenoise_lock();
     }
 }
 
@@ -42,65 +42,107 @@ pub mod rustrt {
     externfn!(fn linenoiseHistoryLoad(file: *c_char) -> c_int)
     externfn!(fn linenoiseSetCompletionCallback(callback: extern "C" fn(*i8, *())))
     externfn!(fn linenoiseAddCompletion(completions: *(), line: *c_char))
+
+    externfn!(fn rust_take_linenoise_lock())
+    externfn!(fn rust_drop_linenoise_lock())
+}
+
+macro_rules! locked {
+    ($expr:expr) => {
+        unsafe {
+            // FIXME #9105: can't use a static mutex in pure Rust yet.
+            rustrt::rust_take_linenoise_lock();
+            let x = $expr;
+            rustrt::rust_drop_linenoise_lock();
+            x
+        }
+    }
 }
 
 /// Add a line to history
-pub unsafe fn add_history(line: &str) -> bool {
+pub fn add_history(line: &str) -> bool {
     do line.with_c_str |buf| {
-        rustrt::linenoiseHistoryAdd(buf) == 1 as c_int
+        (locked!(rustrt::linenoiseHistoryAdd(buf))) == 1 as c_int
     }
 }
 
 /// Set the maximum amount of lines stored
-pub unsafe fn set_history_max_len(len: int) -> bool {
-    rustrt::linenoiseHistorySetMaxLen(len as c_int) == 1 as c_int
+pub fn set_history_max_len(len: int) -> bool {
+    (locked!(rustrt::linenoiseHistorySetMaxLen(len as c_int))) == 1 as c_int
 }
 
 /// Save line history to a file
-pub unsafe fn save_history(file: &str) -> bool {
+pub fn save_history(file: &str) -> bool {
     do file.with_c_str |buf| {
-        rustrt::linenoiseHistorySave(buf) == 1 as c_int
+        // 0 on success, -1 on failure
+        (locked!(rustrt::linenoiseHistorySave(buf))) == 0 as c_int
     }
 }
 
 /// Load line history from a file
-pub unsafe fn load_history(file: &str) -> bool {
+pub fn load_history(file: &str) -> bool {
     do file.with_c_str |buf| {
-        rustrt::linenoiseHistoryLoad(buf) == 1 as c_int
+        // 0 on success, -1 on failure
+        (locked!(rustrt::linenoiseHistoryLoad(buf))) == 0 as c_int
     }
 }
 
 /// Print out a prompt and then wait for input and return it
-pub unsafe fn read(prompt: &str) -> Option<~str> {
+pub fn read(prompt: &str) -> Option<~str> {
     do prompt.with_c_str |buf| {
-        let line = rustrt::linenoise(buf);
+        let line = locked!(rustrt::linenoise(buf));
 
         if line.is_null() { None }
-        else { Some(str::raw::from_c_str(line)) }
+        else {
+            unsafe {
+                do (|| {
+                    Some(str::raw::from_c_str(line))
+                }).finally {
+                    // linenoise's return value is from strdup, so we
+                    // better not leak it.
+                    rt::global_heap::exchange_free(line);
+                }
+            }
+        }
     }
 }
 
 pub type CompletionCb = @fn(~str, @fn(~str));
 
-static complete_key: local_data::Key<@CompletionCb> = &local_data::Key;
-
-/// Bind to the main completion callback
-pub unsafe fn complete(cb: CompletionCb) {
-    local_data::set(complete_key, @cb);
-
-    extern fn callback(line: *c_char, completions: *()) {
-        do local_data::get(complete_key) |cb| {
-            let cb = **cb.unwrap();
-
-            unsafe {
-                do cb(str::raw::from_c_str(line)) |suggestion| {
-                    do suggestion.with_c_str |buf| {
-                        rustrt::linenoiseAddCompletion(completions, buf);
+static complete_key: local_data::Key<CompletionCb> = &local_data::Key;
+
+/// Bind to the main completion callback in the current task.
+///
+/// The completion callback should not call any `extra::rl` functions
+/// other than the closure that it receives as its second
+/// argument. Calling such a function will deadlock on the mutex used
+/// to ensure that the calls are thread-safe.
+pub fn complete(cb: CompletionCb) {
+    local_data::set(complete_key, cb);
+
+    extern fn callback(c_line: *c_char, completions: *()) {
+        do local_data::get(complete_key) |opt_cb| {
+            // only fetch completions if a completion handler has been
+            // registered in the current task.
+            match opt_cb {
+                None => {},
+                Some(cb) => {
+                    let line = unsafe { str::raw::from_c_str(c_line) };
+                    do (*cb)(line) |suggestion| {
+                        do suggestion.with_c_str |buf| {
+                            // This isn't locked, because `callback` gets
+                            // called inside `rustrt::linenoise`, which
+                            // *is* already inside the mutex, so
+                            // re-locking would be a deadlock.
+                            unsafe {
+                                rustrt::linenoiseAddCompletion(completions, buf);
+                            }
+                        }
                     }
                 }
             }
         }
     }
 
-    rustrt::linenoiseSetCompletionCallback(callback);
+    locked!(rustrt::linenoiseSetCompletionCallback(callback));
 }

src/librusti/rusti.rs

@@ -355,12 +355,12 @@ fn compile_crate(src_filename: ~str, binary: ~str) -> Option<bool> {
 /// None if no input was read (e.g. EOF was reached).
 fn get_line(use_rl: bool, prompt: &str) -> Option<~str> {
     if use_rl {
-        let result = unsafe { rl::read(prompt) };
+        let result = rl::read(prompt);
 
         match result {
             None => None,
             Some(line) => {
-                unsafe { rl::add_history(line) };
+                rl::add_history(line);
                 Some(line)
             }
         }
@@ -525,14 +525,12 @@ pub fn main_args(args: &[~str]) {
         println("unstable. If you encounter problems, please use the");
         println("compiler instead. Type :help for help.");
 
-        unsafe {
-            do rl::complete |line, suggest| {
-                if line.starts_with(":") {
-                    suggest(~":clear");
-                    suggest(~":exit");
-                    suggest(~":help");
-                    suggest(~":load");
-                }
+        do rl::complete |line, suggest| {
+            if line.starts_with(":") {
+                suggest(~":clear");
+                suggest(~":exit");
+                suggest(~":help");
+                suggest(~":load");
             }
         }
     }

src/rt/rust_builtin.cpp

@@ -633,6 +633,18 @@ rust_drop_env_lock() {
     env_lock.unlock();
 }
 
+static lock_and_signal linenoise_lock;
+
+extern "C" CDECL void
+rust_take_linenoise_lock() {
+    linenoise_lock.lock();
+}
+
+extern "C" CDECL void
+rust_drop_linenoise_lock() {
+    linenoise_lock.unlock();
+}
+
 extern "C" CDECL unsigned int
 rust_valgrind_stack_register(void *start, void *end) {
   return VALGRIND_STACK_REGISTER(start, end);

src/rt/rustrt.def.in

@@ -189,6 +189,8 @@ rust_take_global_args_lock
 rust_drop_global_args_lock
 rust_take_change_dir_lock
 rust_drop_change_dir_lock
+rust_take_linenoise_lock
+rust_drop_linenoise_lock
 rust_get_test_int
 rust_get_task
 rust_uv_get_loop_from_getaddrinfo_req

src/test/run-pass/rl-human-test.rs

@@ -0,0 +1,75 @@
+// Copyright 2013 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// xfail-fast no compile flags for check-fast
+
+// we want this to be compiled to avoid bitrot, but the actual test
+//has to be conducted by a human, i.e. someone (you?) compiling this
+//file with a plain rustc invocation and running it and checking it
+//works.
+
+// compile-flags: --cfg robot_mode
+
+extern mod extra;
+use extra::rl;
+
+static HISTORY_FILE: &'static str = "rl-human-test-history.txt";
+
+fn main() {
+    // don't run this in robot mode, but still typecheck it.
+    if !cfg!(robot_mode) {
+        println("~~ Welcome to the rl test \"suite\". ~~");
+        println!("Operations:
+ - restrict the history to 2 lines,
+ - set the tab-completion to suggest three copies of each of the last 3 letters (or 'empty'),
+ - add 'one' and 'two' to the history,
+ - save it to `{0}`,
+ - add 'three',
+ - prompt & save input (check the history & completion work and contains only 'two', 'three'),
+ - load from `{0}`
+ - prompt & save input (history should be 'one', 'two' again),
+ - prompt once more.
+
+The bool return values of each step are printed.",
+                 HISTORY_FILE);
+
+        println!("restricting history length: {}", rl::set_history_max_len(3));
+
+        do rl::complete |line, suggest| {
+            if line.is_empty() {
+                suggest(~"empty")
+            } else {
+                for c in line.rev_iter().take(3) {
+                    suggest(format!("{0}{1}{1}{1}", line, c))
+                }
+            }
+        }
+
+        println!("adding 'one': {}", rl::add_history("one"));
+        println!("adding 'two': {}", rl::add_history("two"));
+
+        println!("saving history: {}", rl::save_history(HISTORY_FILE));
+
+        println!("adding 'three': {}", rl::add_history("three"));
+
+        match rl::read("> ") {
+            Some(s) => println!("saving input: {}", rl::add_history(s)),
+            None => return
+        }
+        println!("loading history: {}", rl::load_history(HISTORY_FILE));
+
+        match rl::read("> ") {
+            Some(s) => println!("saving input: {}", rl::add_history(s)),
+            None => return
+        }
+
+        rl::read("> ");
+    }
+}