From 433f38084bba9f18bdb5de22422cbbd5b0c01ff7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= <biwanczuk@gmail.com>
Date: Fri, 25 Nov 2022 02:56:47 +0100
Subject: [PATCH] fix(repl): more reliable history handling (#16797)

This commit changes history handling of the REPL.

There were some situations were history wasn't properly saved and flushed to a
file, making history very spotty. This commit changes it to save every line into
the history file and flush it to disk before being evaluated.

Thanks to this all lines, including "close()" will be stored in the history
file.

If for any reason we're not able to save history file, a single warning will be
printed to the REPL and it will continue to work, even if subsequent tries will
fail to save to disk.
---
 cli/tools/repl/editor.rs | 37 +++++++++++++++++++++++++++----------
 cli/tools/repl/mod.rs    |  7 ++-----
 2 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/cli/tools/repl/editor.rs b/cli/tools/repl/editor.rs
index 69fec9df06..73196d3f34 100644
--- a/cli/tools/repl/editor.rs
+++ b/cli/tools/repl/editor.rs
@@ -5,6 +5,7 @@ use crate::colors;
 use deno_ast::swc::parser::error::SyntaxError;
 use deno_ast::swc::parser::token::Token;
 use deno_ast::swc::parser::token::Word;
+use deno_core::anyhow::Context as _;
 use deno_core::error::AnyError;
 use deno_core::parking_lot::Mutex;
 use deno_core::serde_json;
@@ -27,6 +28,8 @@ use rustyline::{ConditionalEventHandler, Event, EventContext, RepeatCount};
 use rustyline_derive::{Helper, Hinter};
 use std::borrow::Cow;
 use std::path::PathBuf;
+use std::sync::atomic::AtomicBool;
+use std::sync::atomic::Ordering::Relaxed;
 use std::sync::Arc;
 
 use super::channel::RustylineSyncMessageSender;
@@ -368,10 +371,14 @@ impl Highlighter for EditorHelper {
 pub struct ReplEditor {
   inner: Arc<Mutex<Editor<EditorHelper>>>,
   history_file_path: PathBuf,
+  errored_on_history_save: Arc<AtomicBool>,
 }
 
 impl ReplEditor {
-  pub fn new(helper: EditorHelper, history_file_path: PathBuf) -> Self {
+  pub fn new(
+    helper: EditorHelper,
+    history_file_path: PathBuf,
+  ) -> Result<Self, AnyError> {
     let editor_config = Config::builder()
       .completion_type(CompletionType::List)
       .build();
@@ -389,25 +396,35 @@ impl ReplEditor {
       EventHandler::Conditional(Box::new(TabEventHandler)),
     );
 
-    ReplEditor {
+    let history_file_dir = history_file_path.parent().unwrap();
+    std::fs::create_dir_all(history_file_dir).with_context(|| {
+      format!(
+        "Unable to create directory for the history file: {}",
+        history_file_dir.display()
+      )
+    })?;
+
+    Ok(ReplEditor {
       inner: Arc::new(Mutex::new(editor)),
       history_file_path,
-    }
+      errored_on_history_save: Arc::new(AtomicBool::new(false)),
+    })
   }
 
   pub fn readline(&self) -> Result<String, ReadlineError> {
     self.inner.lock().readline("> ")
   }
 
-  pub fn add_history_entry(&self, entry: String) {
+  pub fn update_history(&self, entry: String) {
     self.inner.lock().add_history_entry(entry);
-  }
+    if let Err(e) = self.inner.lock().append_history(&self.history_file_path) {
+      if self.errored_on_history_save.load(Relaxed) {
+        return;
+      }
 
-  pub fn save_history(&self) -> Result<(), AnyError> {
-    std::fs::create_dir_all(self.history_file_path.parent().unwrap())?;
-
-    self.inner.lock().save_history(&self.history_file_path)?;
-    Ok(())
+      self.errored_on_history_save.store(true, Relaxed);
+      eprintln!("Unable to save history file: {}", e);
+    }
   }
 }
 
diff --git a/cli/tools/repl/mod.rs b/cli/tools/repl/mod.rs
index c8efd3d95a..e375915832 100644
--- a/cli/tools/repl/mod.rs
+++ b/cli/tools/repl/mod.rs
@@ -89,7 +89,7 @@ pub async fn run(
   };
 
   let history_file_path = ps.dir.root.join("deno_history.txt");
-  let editor = ReplEditor::new(helper, history_file_path);
+  let editor = ReplEditor::new(helper, history_file_path)?;
 
   if let Some(eval_files) = maybe_eval_files {
     for eval_file in eval_files {
@@ -131,6 +131,7 @@ pub async fn run(
     match line {
       Ok(line) => {
         should_exit_on_interrupt = false;
+        editor.update_history(line.clone());
         let output = repl_session.evaluate_line_and_get_output(&line).await?;
 
         // We check for close and break here instead of making it a loop condition to get
@@ -140,8 +141,6 @@ pub async fn run(
         }
 
         println!("{}", output);
-
-        editor.add_history_entry(line);
       }
       Err(ReadlineError::Interrupted) => {
         if should_exit_on_interrupt {
@@ -161,7 +160,5 @@ pub async fn run(
     }
   }
 
-  editor.save_history()?;
-
   Ok(repl_session.worker.get_exit_code())
 }