From 21ce41d42d6b86e482ec2e7fe01b3feb74dd4ea4 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Wed, 11 Sep 2013 00:50:47 +1000 Subject: extra: use a mutex to wrap linenoise calls and make them threadsafe. Fixes #3921. --- src/libextra/rl.rs | 60 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 18 deletions(-) (limited to 'src/libextra') diff --git a/src/libextra/rl.rs b/src/libextra/rl.rs index 09ef7f22be5..f4ad07d464d 100644 --- a/src/libextra/rl.rs +++ b/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,38 +42,53 @@ 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 + (locked!(rustrt::linenoiseHistorySave(buf))) == 1 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 + (locked!(rustrt::linenoiseHistoryLoad(buf))) == 1 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)) } @@ -84,8 +99,13 @@ 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) { +/// Bind to the main completion callback. +/// +/// 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(line: *c_char, completions: *()) { @@ -95,6 +115,10 @@ pub unsafe fn complete(cb: CompletionCb) { unsafe { do cb(str::raw::from_c_str(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. rustrt::linenoiseAddCompletion(completions, buf); } } @@ -102,5 +126,5 @@ pub unsafe fn complete(cb: CompletionCb) { } } - rustrt::linenoiseSetCompletionCallback(callback); + locked!(rustrt::linenoiseSetCompletionCallback(callback)); } -- cgit 1.4.1-3-g733a5 From fe03d827a4910c561507be0802bb5f39a477cc02 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Wed, 11 Sep 2013 01:04:13 +1000 Subject: extra: stop rl from leaking each line that is read. --- src/libextra/rl.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'src/libextra') diff --git a/src/libextra/rl.rs b/src/libextra/rl.rs index f4ad07d464d..7f0e519f7ba 100644 --- a/src/libextra/rl.rs +++ b/src/libextra/rl.rs @@ -91,7 +91,17 @@ pub fn read(prompt: &str) -> Option<~str> { 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); + } + } + } } } -- cgit 1.4.1-3-g733a5 From 4ed5fcb122870d5b0876b32e2dffeb4ea2bd6aed Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Wed, 11 Sep 2013 21:31:14 +1000 Subject: extra: improvements & bug fixes to rl. - 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. --- src/libextra/rl.rs | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) (limited to 'src/libextra') diff --git a/src/libextra/rl.rs b/src/libextra/rl.rs index 7f0e519f7ba..74b7aea9978 100644 --- a/src/libextra/rl.rs +++ b/src/libextra/rl.rs @@ -74,14 +74,16 @@ pub fn set_history_max_len(len: int) -> bool { /// Save line history to a file pub fn save_history(file: &str) -> bool { do file.with_c_str |buf| { - (locked!(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 fn load_history(file: &str) -> bool { do file.with_c_str |buf| { - (locked!(rustrt::linenoiseHistoryLoad(buf))) == 1 as c_int + // 0 on success, -1 on failure + (locked!(rustrt::linenoiseHistoryLoad(buf))) == 0 as c_int } } @@ -107,29 +109,35 @@ pub fn read(prompt: &str) -> Option<~str> { pub type CompletionCb = @fn(~str, @fn(~str)); -static complete_key: local_data::Key<@CompletionCb> = &local_data::Key; +static complete_key: local_data::Key = &local_data::Key; -/// Bind to the main completion callback. +/// 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(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| { - // This isn't locked, because `callback` gets - // called inside `rustrt::linenoise`, which - // *is* already inside the mutex, so - // re-locking would be a deadlock. - rustrt::linenoiseAddCompletion(completions, buf); + 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); + } + } } } } -- cgit 1.4.1-3-g733a5