[couchdb] branch master updated: Make sure that fsync errors are raised

[da...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

davisp pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/couchdb.git


The following commit(s) were added to refs/heads/master by this push:
     new 3505281  Make sure that fsync errors are raised
3505281 is described below

commit 3505281559513e2922484ebf0996a8846dcc0a34
Author: Paul J. Davis <pa...@gmail.com>
AuthorDate: Sun Jul 21 14:40:52 2019 -0500

    Make sure that fsync errors are raised
    
    This changes `couch_file` to ensure that errors are raised when a call
    to `fsync` fails. It will also stop the couch_file process to ensure
    that anything handling a failed `fsync` won't attempt to retry the
    operation and experience issues discovered by Postgres [1].
    
    [1] http://danluu.com/fsyncgate/
---
 src/couch/src/couch_file.erl        | 29 ++++++++++++++++++++++++++---
 src/couch/test/couch_file_tests.erl | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/src/couch/src/couch_file.erl b/src/couch/src/couch_file.erl
index d6e4066..6db23ea 100644
--- a/src/couch/src/couch_file.erl
+++ b/src/couch/src/couch_file.erl
@@ -215,12 +215,26 @@ truncate(Fd, Pos) ->
 sync(Filepath) when is_list(Filepath) ->
     case file:open(Filepath, [append, raw]) of
         {ok, Fd} ->
-            try ok = file:sync(Fd) after ok = file:close(Fd) end;
+            try
+                case file:sync(Fd) of
+                    ok ->
+                        ok;
+                    {error, Reason} ->
+                        erlang:error({fsync_error, Reason})
+                end
+            after
+                ok = file:close(Fd)
+            end;
         {error, Error} ->
             erlang:error(Error)
     end;
 sync(Fd) ->
-    gen_server:call(Fd, sync, infinity).
+    case gen_server:call(Fd, sync, infinity) of
+        ok ->
+            ok;
+        {error, Reason} ->
+            erlang:error({fsync_error, Reason})
+    end.
 
 %%----------------------------------------------------------------------
 %% Purpose: Close the file.
@@ -462,7 +476,16 @@ handle_call({set_db_pid, Pid}, _From, #file{db_monitor=OldRef}=File) ->
     {reply, ok, File#file{db_monitor=Ref}};
 
 handle_call(sync, _From, #file{fd=Fd}=File) ->
-    {reply, file:sync(Fd), File};
+    case file:sync(Fd) of
+        ok ->
+            {reply, ok, File};
+        {error, _} = Error ->
+            % We're intentionally dropping all knowledge
+            % of this Fd so that we don't accidentally
+            % recover in some whacky edge case that I
+            % can't fathom.
+            {stop, Error, Error, #file{fd = nil}}
+    end;
 
 handle_call({truncate, Pos}, _From, #file{fd=Fd}=File) ->
     {ok, Pos} = file:position(Fd, Pos),
diff --git a/src/couch/test/couch_file_tests.erl b/src/couch/test/couch_file_tests.erl
index 34c1a16..e9806c0 100644
--- a/src/couch/test/couch_file_tests.erl
+++ b/src/couch/test/couch_file_tests.erl
@@ -498,3 +498,36 @@ make_delete_dir_test_case({RootDir, ViewDir}, DeleteAfterRename) ->
 remove_dir(Dir) ->
     [file:delete(File) || File <- filelib:wildcard(filename:join([Dir, "*"]))],
     file:del_dir(Dir).
+
+
+fsync_error_test_() ->
+    {
+        "Test fsync raises errors",
+        {
+            setup,
+            fun() ->
+                test_util:start(?MODULE, [ioq])
+            end,
+            fun(Ctx) ->
+                test_util:stop(Ctx)
+            end,
+            [
+                fun fsync_raises_errors/0
+            ]
+        }
+    }.
+
+
+fsync_raises_errors() ->
+    Fd = spawn(fun() -> fake_fsync_fd() end),
+    ?assertError({fsync_error, eio}, couch_file:sync(Fd)).
+
+
+fake_fsync_fd() ->
+    % Mocking gen_server did not go very
+    % well so faking the couch_file pid
+    % will have to do.
+    receive
+        {'$gen_call', From, sync} ->
+            gen:reply(From, {error, eio})
+    end.