[test] terminate test server when stdin closes

5 files changed, 66 insertions, 46 deletions

diff --git a/test/fixtures/util.cpp b/test/fixtures/util.cpp
index 8c46338817..1a6733e685 100644
--- a/test/fixtures/util.cpp
+++ b/test/fixtures/util.cpp
@@ -28,12 +28,19 @@ std::string getFileSourceRoot() {
 #endif
 }
 
-pid_t startServer(const char *executable) {
-    int pipefd[2];
+Server::Server(const char* executable) {
+    int input[2];
+    int output[2];
 
-    if (pipe(pipefd)) {
-        throw std::runtime_error("Cannot create server pipe");
+    if (pipe(input)) {
+        throw std::runtime_error("Cannot create server input pipe");
     }
+    if (pipe(output)) {
+        throw std::runtime_error("Cannot create server output pipe");
+    }
+
+    // Store the parent => child pipe so that we can close it in the destructor.
+    fd = input[1];
 
     pid_t pid = fork();
     if (pid < 0) {
@@ -42,15 +49,20 @@ pid_t startServer(const char *executable) {
     } else if (pid == 0) {
         // This is the child process.
 
-        // Close the input side of the pipe.
-        close(pipefd[0]);
+        // Connect the parent => child pipe to stdin.
+        while ((dup2(input[0], STDIN_FILENO) == -1) && (errno == EINTR)) {}
+        close(input[0]);
+        close(input[1]);
+
+        // Move the child => parent side of the pipe to stdout.
+        while ((dup2(output[1], STDOUT_FILENO) == -1) && (errno == EINTR)) {}
+        close(output[1]);
+        close(output[0]);
 
         // Launch the actual server process, with the pipe ID as the first argument.
-        char *args[] = { const_cast<char *>(executable),
-                         const_cast<char *>(std::to_string(pipefd[1]).c_str()),
-                         nullptr };
-        int ret = execv(executable, args);
-        // This call should not return. In case execve failed, we exit anyway.
+        int ret = execl(executable, executable);
+
+        // This call should not return. In case execl failed, we exit anyway.
         if (ret < 0) {
             Log::Error(Event::Setup, "Failed to start server: %s", strerror(errno));
         }
@@ -58,30 +70,31 @@ pid_t startServer(const char *executable) {
     } else {
         // This is the parent process.
 
-        // Close output side of the pipe.
-        close(pipefd[1]);
+        // Close the unneeded sides of the pipes.
+        close(output[1]);
+        close(input[0]);
 
-        // Wait until the server process closes the handle.
+        // Wait until the server process sends at least 2 bytes or closes the handle.
         char buffer[2];
-        ssize_t bytes = 0, total = 0;
-        while ((bytes = read(pipefd[0], buffer, sizeof(buffer))) != 0) {
+        ssize_t bytes, total = 0;
+        while (total < 2 && (bytes = read(output[0], buffer + total, 2 - total)) != 0) {
             total += bytes;
         }
-        if (bytes < 0) {
-            Log::Error(Event::Setup, "Failed to start server: %s", strerror(errno));
-            exit(1);
-        }
+
+        // Close child => parent pipe.
+        close(output[0]);
+
+        // Check signature
         if (total != 2 || strncmp(buffer, "OK", 2) != 0) {
-            Log::Error(Event::Setup, "Failed to start server");
-            exit(1);
+            throw std::runtime_error("Failed to start server: Invalid signature");
         }
-        close(pipefd[0]);
     }
-    return pid;
 }
 
-void stopServer(pid_t pid) {
-    kill(pid, SIGTERM);
+Server::~Server() {
+    if (fd > 0) {
+        close(fd);
+    }
 }
 
 // from https://gist.github.com/ArtemGr/997887
diff --git a/test/fixtures/util.hpp b/test/fixtures/util.hpp
index 8dc02eb054..71f7c2a4b3 100644
--- a/test/fixtures/util.hpp
+++ b/test/fixtures/util.hpp
@@ -16,19 +16,25 @@
     } name;
 
 namespace mbgl {
-    
+
 class Map;
-    
+
 namespace test {
-    
+
 std::string getFileSourceRoot();
 
-pid_t startServer(const char *executable);
-void stopServer(pid_t pid);
+class Server {
+public:
+    Server(const char* executable);
+    ~Server();
+
+private:
+    int fd = -1;
+};
 
 uint64_t crc64(const char*, size_t);
 uint64_t crc64(const std::string&);
-    
+
 PremultipliedImage render(Map&);
 
 void checkImage(const std::string& base,
diff --git a/test/storage/server.js b/test/storage/server.js
index 249c6a124b..a814ce15e6 100755
--- a/test/storage/server.js
+++ b/test/storage/server.js
@@ -2,6 +2,13 @@
 /* jshint node: true */
 'use strict';
 
+// This needs to be here to make sure the pipe stays open.
+// We're waiting until the stdin pipe gets closed (e.g. because the parent
+// process dies)
+process.stdin.on('readable', function() {});
+process.stdin.on('end', function() { process.exit(0); });
+
+
 var fs = require('fs');
 var express = require('express');
 var app = express();
@@ -101,13 +108,6 @@ app.get('/load/:number(\\d+)', function(req, res) {
 });
 
 var server = app.listen(3000, function () {
-    var host = server.address().address;
-    var port = server.address().port;
-    console.warn('Storage test server listening at http://%s:%s', host, port);
-
-    if (process.argv[2]) {
-        // Allow the test to continue running.
-        fs.write(+process.argv[2], 'OK');
-        fs.close(+process.argv[2]);
-    }
+    // Tell parent that we're now listening.
+    process.stdout.write("OK");
 });
diff --git a/test/storage/storage.cpp b/test/storage/storage.cpp
index 7fafe59e47..91f21107ad 100644
--- a/test/storage/storage.cpp
+++ b/test/storage/storage.cpp
@@ -2,13 +2,13 @@
 
 #include <mbgl/platform/platform.hpp>
 
-pid_t Storage::pid = 0;
+std::unique_ptr<mbgl::test::Server> Storage::server;
 
 void Storage::SetUpTestCase() {
-    const auto server = mbgl::platform::applicationRoot() + "/TEST_DATA/storage/server.js";
-    pid = mbgl::test::startServer(server.c_str());
+    const auto program = mbgl::platform::applicationRoot() + "/TEST_DATA/storage/server.js";
+    server = std::make_unique<mbgl::test::Server>(program.c_str());
 }
 
 void Storage::TearDownTestCase() {
-    mbgl::test::stopServer(pid);
+    server.reset();
 }
diff --git a/test/storage/storage.hpp b/test/storage/storage.hpp
index 34fa69fbf9..dc304e6b6f 100644
--- a/test/storage/storage.hpp
+++ b/test/storage/storage.hpp
@@ -4,6 +4,7 @@
 #include "../fixtures/util.hpp"
 #include <mbgl/storage/response.hpp>
 #include <iostream>
+#include <memory>
 
 class Storage : public testing::Test {
 public:
@@ -11,7 +12,7 @@ public:
     static void TearDownTestCase();
 
 protected:
-    static pid_t pid;
+    static std::unique_ptr<mbgl::test::Server> server;
 };
 
 namespace mbgl {