Merge branch 'pan/fdmanana/fix_efile_drv_crash'

* pan/fdmanana/fix_efile_drv_crash:
  Fix efile_drv crash when using async thread pool
  Alternative solution to the efile_drv crash on exit

OTP-10748

2 files changed, 123 insertions, 27 deletions

diff --git a/erts/emulator/drivers/common/efile_drv.c b/erts/emulator/drivers/common/efile_drv.c
index 25b02db2c9..186af03eff 100644
--- a/erts/emulator/drivers/common/efile_drv.c
+++ b/erts/emulator/drivers/common/efile_drv.c
@@ -57,7 +57,7 @@
 #define FILE_FADVISE            31
 #define FILE_SENDFILE           32
 #define FILE_FALLOCATE          33
-
+#define FILE_CLOSE_ON_PORT_EXIT 34
 /* Return codes */
 
 #define FILE_RESP_OK         0
@@ -178,6 +178,7 @@ dt_private *get_dt_private(int);
 #define MUTEX_LOCK(m)    do { IF_THRDS { TRACE_DRIVER; driver_pdl_lock(m);   } } while (0)
 #define MUTEX_UNLOCK(m)  do { IF_THRDS { TRACE_DRIVER; driver_pdl_unlock(m); } } while (0)
 #else
+#define IF_THRDS if (0)
 #define MUTEX_INIT(m, p)
 #define MUTEX_LOCK(m)
 #define MUTEX_UNLOCK(m)
@@ -429,6 +430,7 @@ struct t_data
     int            level;
     void         (*invoke)(void *);
     void         (*free)(void *);
+    void           *data_to_free; /* used by FILE_CLOSE_ON_PORT_EXIT only */
     int            again;
     int            reply;
 #ifdef  USE_VM_PROBES
@@ -808,34 +810,25 @@ static void free_data(void *data)
 {
     struct t_data *d = (struct t_data *) data;
 
-    if (d->command == FILE_OPEN && d->is_fd_unused && d->fd != FILE_FD_INVALID) {
-        do_close(d->flags, d->fd);
+    switch (d->command) {
+    case FILE_OPEN:
+        if (d->is_fd_unused && d->fd != FILE_FD_INVALID) {
+            /* This is OK to do in scheduler thread because there can be no async op
+               ongoing for this fd here, as we exited during async open.
+               Ideally, this close should happen in an async thread too, but that would
+               require a substantial rewrite, as we are here because of a dead port and
+               cannot schedule async jobs for that port any more... */
+            do_close(d->flags, d->fd);
+        }
+        break;
+    case FILE_CLOSE_ON_PORT_EXIT:
+        EF_FREE(d->data_to_free);
+        break;
     }
 
     EF_FREE(data);
 }
 
-/*********************************************************************
- * Driver entry point -> stop
- */
-static void 
-file_stop(ErlDrvData e)
-{
-    file_descriptor* desc = (file_descriptor*)e;
-
-    TRACE_C('p');
-
-    if (desc->fd != FILE_FD_INVALID) {
-	do_close(desc->flags, desc->fd);
-	desc->fd = FILE_FD_INVALID;
-	desc->flags = 0;
-    }
-    if (desc->read_binp) {
-	driver_free_binary(desc->read_binp);
-    }
-    EF_FREE(desc);
-}
-
 
 /*
  * Sends back an error reply to Erlang.
@@ -2242,6 +2235,47 @@ static int lseek_flush_read(file_descriptor *desc, int *errp
 }
 
 
+/*********************************************************************
+ * Driver entry point -> stop
+ * The close has to be scheduled on async thread, so that currently active
+ * async operation does not suddenly have the ground disappearing under their feet...
+ */
+static void 
+file_stop(ErlDrvData e)
+{
+    file_descriptor* desc = (file_descriptor*)e;
+
+    TRACE_C('p');
+
+    IF_THRDS {
+	flush_read(desc);
+	if (desc->fd != FILE_FD_INVALID) {
+	    struct t_data *d = EF_SAFE_ALLOC(sizeof(struct t_data));
+	    d->command = FILE_CLOSE_ON_PORT_EXIT;
+	    d->reply = !0;
+	    d->fd = desc->fd;
+	    d->flags = desc->flags;
+	    d->invoke = invoke_close;
+	    d->free = free_data;
+	    d->level = 2;
+	    d->data_to_free = (void *) desc;
+	    cq_enq(desc, d);
+	    desc->fd = FILE_FD_INVALID;
+	    desc->flags = 0;
+	    cq_execute(desc);
+	}
+    } else {
+	if (desc->fd != FILE_FD_INVALID) {
+	    do_close(desc->flags, desc->fd);
+	    desc->fd = FILE_FD_INVALID;
+	    desc->flags = 0;
+	}
+	if (desc->read_binp) {
+	    driver_free_binary(desc->read_binp);
+	}
+	EF_FREE(desc);
+    }
+}
 
 /*********************************************************************
  * Driver entry point -> ready_async
@@ -2465,7 +2499,6 @@ file_async_ready(ErlDrvData e, ErlDrvThreadData data)
 	}
 	free_readdir(data);
 	break;
-	/* See file_stop */
       case FILE_CLOSE:
 	  if (d->reply) {
 	      TRACE_C('K');
@@ -2525,6 +2558,15 @@ file_async_ready(ErlDrvData e, ErlDrvThreadData data)
 	  }
 	  break;
 #endif
+      case FILE_CLOSE_ON_PORT_EXIT:
+	  /* See file_stop. However this is never invoked after the port is killed. */
+	  free_data(data);
+	  EF_FREE(desc);
+	  desc = NULL;
+	  /* This is it for this port, so just send dtrace and return, avoid doing anything to the freed data */
+	  DTRACE6(efile_drv_return, sched_i1, sched_i2, sched_utag,
+		  command, result_ok, posix_errno);
+	  return;
       default:
 	abort();
     }
@@ -2535,6 +2577,7 @@ file_async_ready(ErlDrvData e, ErlDrvThreadData data)
 	driver_set_timer(desc->port, desc->write_delay);
     }
     cq_execute(desc);
+
 }
 
 
diff --git a/lib/kernel/test/file_SUITE.erl b/lib/kernel/test/file_SUITE.erl
index 914f0d6127..f34341f561 100644
--- a/lib/kernel/test/file_SUITE.erl
+++ b/lib/kernel/test/file_SUITE.erl
@@ -60,7 +60,8 @@
 -export([ read_not_really_compressed/1,
 	 read_compressed_cooked/1, read_compressed_cooked_binary/1,
 	 read_cooked_tar_problem/1,
-	 write_compressed/1, compress_errors/1, catenated_gzips/1]).
+	 write_compressed/1, compress_errors/1, catenated_gzips/1,
+	 compress_async_crash/1]).
 
 -export([ make_link/1, read_link_info_for_non_link/1, symlinks/1]).
 
@@ -135,7 +136,8 @@ groups() ->
      {compression, [],
       [read_compressed_cooked, read_compressed_cooked_binary,
        read_cooked_tar_problem, read_not_really_compressed,
-       write_compressed, compress_errors, catenated_gzips]},
+       write_compressed, compress_errors, catenated_gzips,
+       compress_async_crash]},
      {links, [],
       [make_link, read_link_info_for_non_link, symlinks]}].
 
@@ -2312,6 +2314,57 @@ compress_errors(Config) when is_list(Config) ->
 
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 
+compress_async_crash(suite) -> [];
+compress_async_crash(doc) -> [];
+compress_async_crash(Config) when is_list(Config) ->
+    ?line DataDir = ?config(data_dir, Config),
+    ?line Path = filename:join(DataDir, "test.gz"),
+    ExpectedData = <<"qwerty">>,
+
+    ?line _ = ?FILE_MODULE:delete(Path),
+    ?line {ok, Fd} = ?FILE_MODULE:open(Path, [write, binary, compressed]),
+    ?line ok = ?FILE_MODULE:write(Fd, ExpectedData),
+    ?line ok = ?FILE_MODULE:close(Fd),
+
+    % Test that when using async thread pool, the emulator doesn't crash
+    % when the efile port driver is stopped while a compressed file operation
+    % is in progress (being carried by an async thread).
+    ?line ok = compress_async_crash_loop(10000, Path, ExpectedData),
+    ?line ok = ?FILE_MODULE:delete(Path),
+    ok.
+
+compress_async_crash_loop(0, _Path, _ExpectedData) ->
+    ok;
+compress_async_crash_loop(N, Path, ExpectedData) ->
+    Parent = self(),
+    {Pid, Ref} = spawn_monitor(
+            fun() ->
+                    ?line {ok, Fd} = ?FILE_MODULE:open(
+                                        Path, [read, compressed, raw, binary]),
+                    Len = byte_size(ExpectedData),
+                    Parent ! {self(), continue},
+                    ?line {ok, ExpectedData} = ?FILE_MODULE:read(Fd, Len),
+                    ?line ok = ?FILE_MODULE:close(Fd),
+                    receive foobar -> ok end
+            end),
+    receive
+        {Pid, continue} ->
+            exit(Pid, shutdown),
+            receive
+                {'DOWN', Ref, _, _, Reason} ->
+                    ?line shutdown = Reason
+            end;
+        {'DOWN', Ref, _, _, Reason2} ->
+            test_server:fail({worker_exited, Reason2})
+    after 60000 ->
+            exit(Pid, shutdown),
+            erlang:demonitor(Ref, [flush]),
+            test_server:fail(worker_timeout)
+    end,
+    compress_async_crash_loop(N - 1, Path, ExpectedData).
+
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+
 altname(doc) ->
     "Test the file:altname/1 function";
 altname(suite) ->