From 59fe124c40514ff22602397002d23c64fcb1ee8e Mon Sep 17 00:00:00 2001
From: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Sun, 19 Feb 2023 16:50:15 -0600
Subject: [PATCH] builtins/random: Don't lock the mutex unnecessarily

The mutex was being locked from the very start, before it was needed and
possibly before it would be needed.

Also rename the static global to stick to rust naming conventions.

Note that `once_cell::sync::Lazy<T>` actually internally uses its own lock
around the value, but in this case it's insufficient because `SmallRng` doesn't
implement `SeedableRng` so we can't reseed it with only an `&mut` reference and
must instead replace its value.

We probably *could* still use `Lazy<SmallRng>` directly and then rely on
`std::mem::swap()` to replace the contents of the shared global static without
reassigning the variable directly with a new `SmallRng` instance, but I'm not
sure that's a great idea. This is just a built-in, there's no real harm in
locking twice (especially while fish remains essentially single-threaded).
---
 fish-rust/src/builtins/random.rs | 34 ++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/fish-rust/src/builtins/random.rs b/fish-rust/src/builtins/random.rs
index 22314774f..bcded00c5 100644
--- a/fish-rust/src/builtins/random.rs
+++ b/fish-rust/src/builtins/random.rs
@@ -5,7 +5,7 @@ use crate::builtins::shared::{
     STATUS_CMD_OK, STATUS_INVALID_ARGS,
 };
 use crate::ffi::parser_t;
-use crate::wchar::{widestrs, wstr};
+use crate::wchar::{wstr, L};
 use crate::wgetopt::{wgetopter_t, wopt, woption, woption_argument_t};
 use crate::wutil::{self, fish_wcstoi_radix_all, format::printf::sprintf, wgettext_fmt};
 use num_traits::PrimInt;
@@ -14,9 +14,8 @@ use rand::rngs::SmallRng;
 use rand::{Rng, SeedableRng};
 use std::sync::Mutex;
 
-static seeded_engine: Lazy<Mutex<SmallRng>> = Lazy::new(|| Mutex::new(SmallRng::from_entropy()));
+static RNG: Lazy<Mutex<SmallRng>> = Lazy::new(|| Mutex::new(SmallRng::from_entropy()));
 
-#[widestrs]
 pub fn random(
     parser: &mut parser_t,
     streams: &mut io_streams_t,
@@ -26,8 +25,8 @@ pub fn random(
     let argc = argv.len();
     let print_hints = false;
 
-    const shortopts: &wstr = "+:h"L;
-    const longopts: &[woption] = &[wopt("help"L, woption_argument_t::no_argument, 'h')];
+    const shortopts: &wstr = L!("+:h");
+    const longopts: &[woption] = &[wopt(L!("help"), woption_argument_t::no_argument, 'h')];
 
     let mut w = wgetopter_t::new(shortopts, longopts, argv);
     while let Some(c) = w.wgetopt_long() {
@@ -50,7 +49,6 @@ pub fn random(
         }
     }
 
-    let mut engine = seeded_engine.lock().unwrap();
     let mut start = 0;
     let mut end = 32767;
     let mut step = 1;
@@ -64,8 +62,10 @@ pub fn random(
             return STATUS_INVALID_ARGS;
         }
 
-        let rand = engine.gen_range(0..arg_count - 1);
-        streams.out.append(sprintf!("%ls\n"L, argv[i + 1 + rand]));
+        let rand = RNG.lock().unwrap().gen_range(0..arg_count - 1);
+        streams
+            .out
+            .append(sprintf!(L!("%ls\n"), argv[i + 1 + rand]));
         return STATUS_CMD_OK;
     }
     fn parse<T: PrimInt>(
@@ -91,7 +91,10 @@ pub fn random(
             let num = parse::<i64>(streams, cmd, argv[i]);
             match num {
                 Err(_) => return STATUS_INVALID_ARGS,
-                Ok(x) => *engine = SmallRng::seed_from_u64(x as u64),
+                Ok(x) => {
+                    let mut engine = RNG.lock().unwrap();
+                    *engine = SmallRng::seed_from_u64(x as u64);
+                }
             }
             return STATUS_CMD_OK;
         }
@@ -156,12 +159,17 @@ pub fn random(
         return STATUS_INVALID_ARGS;
     }
 
-    let rand = engine.gen_range(0..=possibilities);
+    let rand = {
+        let mut engine = RNG.lock().unwrap();
+        engine.gen_range(0..=possibilities)
+    };
 
     // Safe because end was a valid i64 and the result here is in the range start..=end.
-    let result: i64 = start.checked_add_unsigned(rand * step)
-        .and_then(|x| x.try_into().ok()).unwrap();
+    let result: i64 = start
+        .checked_add_unsigned(rand * step)
+        .and_then(|x| x.try_into().ok())
+        .unwrap();
 
-    streams.out.append(sprintf!("%d\n"L, result));
+    streams.out.append(sprintf!(L!("%d\n"), result));
     return STATUS_CMD_OK;
 }