Large set of changes to improve history atomicity on filesystems that do not support locking

Should address https://github.com/fish-shell/fish-shell/issues/685

fish_tests.cpp

@@ -1668,13 +1668,13 @@ void history_tests_t::test_history_formats(void)
 
 void history_tests_t::test_history_speed(void)
 {
-    say(L"Testing history speed");
+    say(L"Testing history speed (pid is %d)", getpid());
     history_t *hist = new history_t(L"speed_test");
     wcstring item = L"History Speed Test - X";
 
     /* Test for 10 seconds */
     double start = timef();
-    double end = start + 4;
+    double end = start + 10;
     double stop = 0;
     size_t count = 0;
     for (;;)
@@ -1715,7 +1715,6 @@ int main(int argc, char **argv)
     reader_init();
     env_init();
     
-
     test_format();
     test_escape();
     test_convert();

history.cpp

@@ -47,20 +47,82 @@ Our history format is intended to be valid YAML. Here it is:
   Newlines are replaced by \n. Backslashes are replaced by \\.
 */
 
-
-
 /** When we rewrite the history, the number of items we keep */
 #define HISTORY_SAVE_MAX (1024 * 256)
 
-/** Interval in seconds between automatic history save */
-#define SAVE_INTERVAL (5*60)
-
-/** Number of new history entries to add before automatic history save */
-#define SAVE_COUNT 5
-
 /** Whether we print timing information */
 #define LOG_TIMES 0
 
+/** Default buffer size for flushing to the history file */
+#define HISTORY_OUTPUT_BUFFER_SIZE (4096 * 4)
+
+/* Helper class for certain output. This is basically a string that allows us to ensure we only flush at record boundaries, and avoids the copying of ostringstream. Have you ever tried to implement your own streambuf? Total insanity. */
+class history_output_buffer_t
+{
+    /* A null-terminated C string */
+    std::vector<char> buffer;
+
+    /* Offset is the offset of the null terminator */
+    size_t offset;
+
+    static size_t safe_strlen(const char *s)
+    {
+        return s ? strlen(s) : 0;
+    }
+
+    public:
+
+    /* Add a bit more to HISTORY_OUTPUT_BUFFER_SIZE because we flush once we've exceeded that size */
+    history_output_buffer_t() : buffer(HISTORY_OUTPUT_BUFFER_SIZE + 128, '\0'), offset(0)
+    {
+    }
+
+    /* Append one or more strings */
+    void append(const char *s1, const char *s2 = NULL, const char *s3 = NULL)
+    {
+        const char *ptrs[4] = {s1, s2, s3, NULL};
+        const size_t lengths[4] = {safe_strlen(s1), safe_strlen(s2), safe_strlen(s3), 0};
+
+        /* Determine the additional size we'll need */
+        size_t additional_length = 0;
+        for (size_t i=0; i < sizeof lengths / sizeof *lengths; i++)
+        {
+            additional_length += lengths[i];
+        }
+
+        /* Allocate that much, plus a null terminator */
+        size_t required_size = offset + additional_length + 1;
+        if (required_size > buffer.size())
+        {
+            buffer.resize(required_size, '\0');
+        }
+
+        /* Copy */
+        for (size_t i=0; ptrs[i] != NULL; i++)
+        {
+            memmove(&buffer.at(offset), ptrs[i], lengths[i]);
+            offset += lengths[i];
+        }
+
+        /* Null terminator was appended by virtue of the resize() above (or in a previous invocation). */
+        assert(buffer.at(buffer.size() - 1) == '\0');
+    }
+
+    /* Output to a given fd, resetting our buffer. Returns true on success, false on error */
+    bool flush_to_fd(int fd)
+    {
+        bool result = write_loop(fd, &buffer.at(0), offset) >= 0;
+        offset = 0;
+        return result;
+    }
+
+    /* Return how much data we've accumulated */
+    size_t output_size() const
+    {
+        return offset;
+    }
+};
+
 class time_profiler_t
 {
     const char *what;
@@ -216,32 +278,28 @@ bool history_item_t::matches_search(const wcstring &term, enum history_search_ty
     }
 }
 
-/* Output our YAML history format to a file. */
+/* Append our YAML history format to the provided vector at the given offset, updating the offset */
-static bool write_yaml_to_file(const wcstring &wcmd, time_t timestamp, const path_list_t &required_paths, FILE *f)
+static void append_yaml_to_buffer(const wcstring &wcmd, time_t timestamp, const path_list_t &required_paths, history_output_buffer_t *buffer)
 {
     std::string cmd = wcs2string(wcmd);
     escape_yaml(cmd);
-    if (fprintf(f, "- cmd: %s\n", cmd.c_str()) < 0)
+    buffer->append("- cmd: ", cmd.c_str(), "\n");
-        return false;
 
-    if (fprintf(f, "   when: %ld\n", (long)timestamp) < 0)
+    char timestamp_str[96];
-        return false;
+    snprintf(timestamp_str, sizeof timestamp_str, "%ld", timestamp);
+    buffer->append("   when: ", timestamp_str, "\n");
 
     if (! required_paths.empty())
     {
-        if (fputs("   paths:\n", f) < 0)
+        buffer->append("   paths:\n");
-            return false;
 
         for (path_list_t::const_iterator iter = required_paths.begin(); iter != required_paths.end(); ++iter)
         {
             std::string path = wcs2string(*iter);
             escape_yaml(path);
-
+            buffer->append("    - ", path.c_str(), "\n");
-            if (fprintf(f, "    - %s\n", path.c_str()) < 0)
-                return false;
         }
     }
-    return true;
 }
 
 // Parse a timestamp line that looks like this: spaces, "when:", spaces, timestamp, newline
@@ -913,7 +971,7 @@ void history_t::populate_from_mmap(void)
 }
 
 /* Do a private, read-only map of the entirety of a history file with the given name. Returns true if successful. Returns the mapped memory region by reference. */
-static bool map_file(const wcstring &name, const char **out_map_start, size_t *out_map_len, file_id_t *file_id)
+bool history_t::map_file(const wcstring &name, const char **out_map_start, size_t *out_map_len, file_id_t *file_id)
 {
     bool result = false;
     wcstring filename = history_filename(name, L"");
@@ -927,22 +985,25 @@ static bool map_file(const wcstring &name, const char **out_map_start, size_t *o
             if (file_id != NULL)
                 *file_id = history_file_identify(fd);
 
-            /* Take a read lock to guard against someone else appending. This is released when the file is closed (below). We will read the file after taking the lock, but that's not a problem, because we never modify already written data. In short, the purpose of this lock is to ensure we don't see the file size change mid-update. */
+            /* Take a read lock to guard against someone else appending. This is released when the file is closed (below). We will read the file after releasing the lock, but that's not a problem, because we never modify already written data. In short, the purpose of this lock is to ensure we don't see the file size change mid-update.
-            if (history_file_lock(fd, F_RDLCK))
+
+                We may fail to lock (e.g. on lockless NFS - see https://github.com/fish-shell/fish-shell/issues/685 ). In that case, we proceed as if it did not fail. The risk is that we may get an incomplete history item; this is unlikely because we only treat an item as valid if it has a terminating newline.
+
+                Simulate a failing lock in chaos_mode
+            */
+            if (! chaos_mode) history_file_lock(fd, F_RDLCK);
+            off_t len = lseek(fd, 0, SEEK_END);
+            if (len != (off_t)-1)
             {
-                off_t len = lseek(fd, 0, SEEK_END);
+                size_t mmap_length = (size_t)len;
-                if (len != (off_t)-1)
+                if (lseek(fd, 0, SEEK_SET) == 0)
                 {
-                    size_t mmap_length = (size_t)len;
+                    char *mmap_start;
-                    if (lseek(fd, 0, SEEK_SET) == 0)
+                    if ((mmap_start = (char *)mmap(0, mmap_length, PROT_READ, MAP_PRIVATE, fd, 0)) != MAP_FAILED)
                     {
-                        char *mmap_start;
+                        result = true;
-                        if ((mmap_start = (char *)mmap(0, mmap_length, PROT_READ, MAP_PRIVATE, fd, 0)) != MAP_FAILED)
+                        *out_map_start = mmap_start;
-                        {
+                        *out_map_len = mmap_length;
-                            result = true;
-                            *out_map_start = mmap_start;
-                            *out_map_len = mmap_length;
-                        }
                     }
                 }
             }
@@ -1188,7 +1249,7 @@ bool history_t::save_internal_via_rewrite()
     /* This must be called while locked */
     ASSERT_IS_LOCKED(lock);
 
-    bool ok = true;
+    bool ok = false;
 
     wcstring tmp_name_template = history_filename(name, L".XXXXXX");
     if (! tmp_name_template.empty())
@@ -1266,53 +1327,40 @@ bool history_t::save_internal_via_rewrite()
 
         if (out_fd >= 0)
         {
-            /* Success */
+            /* Write them out */
-            FILE *out = fdopen(out_fd, "w");
+            bool errored = false;
-            if (out)
+            history_output_buffer_t buffer;
+            for (history_lru_cache_t::iterator iter = lru.begin(); iter != lru.end(); ++iter)
             {
-                /* Be block buffered. In chaos mode, choose a tiny buffer so as to magnify the effects of race conditions. Otherwise, use the default buffer */
+                const history_lru_node_t *node = *iter;
-                setvbuf(out, NULL, _IOFBF, chaos_mode ? 1 : 0);
+                append_yaml_to_buffer(node->key, node->timestamp, node->required_paths, &buffer);
-
+                if (buffer.output_size() >= HISTORY_OUTPUT_BUFFER_SIZE && ! buffer.flush_to_fd(out_fd))
-                /* Write them out */
-                for (history_lru_cache_t::iterator iter = lru.begin(); iter != lru.end(); ++iter)
                 {
-                    const history_lru_node_t *node = *iter;
+                    errored = true;
-                    if (! write_yaml_to_file(node->key, node->timestamp, node->required_paths, out))
+                    break;
-                    {
-                        ok = false;
-                        break;
-                    }
-                }
-
-                if (0 == fclose(out))
-                {
-                    /* fclose closed out_fd, so mark it as -1 so we don't try to close it later */
-                    out_fd = -1;
-                }
-                else
-                {
-                    /* error on close */
-                    ok = false;
-                }
-
-                if (! ok)
-                {
-                    /*
-                      This message does not have high enough priority to
-                      be shown by default.
-                    */
-                    debug(2, L"Error when writing history file");
-                }
-                else
-                {
-                    wcstring new_name = history_filename(name, wcstring());
-                    wrename(tmp_name, new_name);
                 }
             }
-        }
 
-        if (out_fd >= 0)
+            if (! errored && buffer.flush_to_fd(out_fd))
+            {
+                ok = true;
+            }
+
+            if (! ok)
+            {
+                /*
+                  This message does not have high enough priority to
+                  be shown by default.
+                */
+                debug(2, L"Error when writing history file");
+            }
+            else
+            {
+                wcstring new_name = history_filename(name, wcstring());
+                wrename(tmp_name, new_name);
+            }
             close(out_fd);
+        }
 
         signal_unblock();
 
@@ -1362,61 +1410,49 @@ bool history_t::save_internal_via_appending()
         if (history_file_identify(out_fd) != mmap_file_id)
             file_changed = true;
 
-        /* Exclusive lock on the entire file. This is released when we close the file (below). */
+        /* Exclusive lock on the entire file. This is released when we close the file (below). This may fail on (e.g.) lockless NFS. If so, proceed as if it did not fail; the risk is that we may get interleaved history items, which is considered better than no history, or forcing everything through the slow copy-move mode. We try to minimize this possibility by writing with O_APPEND.
-        if (history_file_lock(out_fd, F_WRLCK))
+
+           Simulate a failing lock in chaos_mode
+        */
+        if (! chaos_mode) history_file_lock(out_fd, F_WRLCK);
+
+        /* We (hopefully successfully) took the exclusive lock. Append to the file.
+           Note that this is sketchy for a few reasons:
+             - Another shell may have appended its own items with a later timestamp, so our file may no longer be sorted by timestamp.
+             - Another shell may have appended the same items, so our file may now contain duplicates.
+
+           We cannot modify any previous parts of our file, because other instances may be reading those portions. We can only append.
+
+           Originally we always rewrote the file on saving, which avoided both of these problems. However, appending allows us to save history after every command, which is nice!
+
+           Periodically we "clean up" the file by rewriting it, so that most of the time it doesn't have duplicates, although we don't yet sort by timestamp (the timestamp isn't really used for much anyways).
+        */
+
+        /* So far so good. Write all items at or after first_unwritten_new_item_index */
+        
+        bool errored = false;
+        history_output_buffer_t buffer;
+        while (first_unwritten_new_item_index < new_items.size())
         {
-            /* We successfully took the exclusive lock. Append to the file.
+            const history_item_t &item = new_items.at(first_unwritten_new_item_index);
-               Note that this is sketchy for a few reasons:
+            append_yaml_to_buffer(item.str(), item.timestamp(), item.get_required_paths(), &buffer);
-                 - Another shell may have appended its own items with a later timestamp, so our file may no longer be sorted by timestamp.
+            if (buffer.output_size() >= HISTORY_OUTPUT_BUFFER_SIZE)
-                 - Another shell may have appended the same items, so our file may now contain duplicates.
-
-               We cannot modify any previous parts of our file, because other instances may be reading those portions. We can only append.
-
-               Originally we always rewrote the file on saving, which avoided both of these problems. However, appending allows us to save history after every command, which is nice!
-
-               Periodically we "clean up" the file by rewriting it, so that most of the time it doesn't have duplicates, although we don't yet sort by timestamp (the timestamp isn't really used for much anyways).
-            */
-
-            FILE *out = fdopen(out_fd, "a");
-            if (out)
             {
-                /* Be block buffered. In chaos mode, choose a tiny buffer so as to magnify the effects of race conditions. Otherwise, use the default buffer */
+                errored = ! buffer.flush_to_fd(out_fd);
-                setvbuf(out, NULL, _IOFBF, chaos_mode ? 1 : 0);
+                if (errored) break;
-
-                bool errored = false;
-
-                /* Write all items at or after first_unwritten_new_item_index */
-                while (first_unwritten_new_item_index < new_items.size())
-                {
-                    const history_item_t &item = new_items.at(first_unwritten_new_item_index);
-                    if (! write_yaml_to_file(item.str(), item.timestamp(), item.get_required_paths(), out))
-                    {
-                        errored = true;
-                        break;
-                    }
-
-                    /* We wrote this item, hooray */
-                    first_unwritten_new_item_index++;
-                }
-
-                if (0 == fclose(out))
-                {
-                    /* fclose just closed our out_fd; mark it as -1 so we don't re-close it */
-                    out_fd = -1;
-                }
-                else
-                {
-                    errored = true;
-                }
-
-                /* We're OK if we did not error */
-                ok = ! errored;
             }
-        }
-    }
 
-    if (out_fd >= 0)
+            /* We wrote this item, hooray */
+            first_unwritten_new_item_index++;
+        }
+
+        if (! errored && buffer.flush_to_fd(out_fd))
+        {
+            ok = true;
+        }
+
         close(out_fd);
+    }
 
     signal_unblock();
 

history.h

@@ -175,6 +175,9 @@ private:
     /** Saves history */
     void save_internal(bool vacuum);
 
+    /* Do a private, read-only map of the entirety of a history file with the given name. Returns true if successful. Returns the mapped memory region by reference. */
+    bool map_file(const wcstring &name, const char **out_map_start, size_t *out_map_len, file_id_t *file_id);
+
     /** Whether we're in maximum chaos mode, useful for testing */
     bool chaos_mode;