From 758b5f2e4628e5ad08d75a4685408e04160ec590 Mon Sep 17 00:00:00 2001 From: Oliver Giles Date: Fri, 6 Jul 2018 13:18:04 +0300 Subject: [PATCH] resolves #37: closed connection on large files The old implementation slurped the whole artefact into memory, and did not ensure it remained allocated beyond the first call to write(). The new implementation uses mmap and ensures the mapping lasts until the file has been delivered --- src/interface.h | 15 +++++++++++---- src/laminar.cpp | 40 ++++++++++++++++++++++++++++++++++------ src/laminar.h | 2 +- src/server.cpp | 11 +++++------ test/test-server.cpp | 5 ++++- 5 files changed, 55 insertions(+), 18 deletions(-) diff --git a/src/interface.h b/src/interface.h index fa83cec..6b67f63 100644 --- a/src/interface.h +++ b/src/interface.h @@ -80,11 +80,18 @@ struct LaminarWaiter { virtual void complete(const Run*) = 0; }; +// Represents a file mapped in memory. Used to serve artefacts +struct MappedFile { + virtual ~MappedFile() =default; + virtual const void* address() = 0; + virtual size_t size() = 0; +}; + // The interface connecting the network layer to the application business // logic. These methods fulfil the requirements of both the HTTP/Websocket // and RPC interfaces. struct LaminarInterface { - virtual ~LaminarInterface() =default; + virtual ~LaminarInterface() {} // Queues a job, returns immediately. Return value will be nullptr if // the supplied name is not a known job. @@ -118,9 +125,9 @@ struct LaminarInterface { virtual bool setParam(std::string job, uint buildNum, std::string param, std::string value) = 0; // Fetches the content of an artifact given its filename relative to - // $LAMINAR_HOME/archive. This shouldn't be used, because the sysadmin - // should have configured a real webserver to serve these things. - virtual bool getArtefact(std::string path, std::string& result) = 0; + // $LAMINAR_HOME/archive. Ideally, this would instead be served by a + // proper web server which handles this url. + virtual kj::Own getArtefact(std::string path) = 0; // Fetches the content of $LAMINAR_HOME/custom/style.css or an empty // string. This shouldn't be used, because the sysadmin should have diff --git a/src/laminar.cpp b/src/laminar.cpp index 09f8fd6..6abef4f 100644 --- a/src/laminar.cpp +++ b/src/laminar.cpp @@ -22,6 +22,10 @@ #include "log.h" #include +#include +#include +#include +#include #include #include @@ -840,13 +844,37 @@ static bool slurp(fs::path path, std::string& output) { return false; } -bool Laminar::getArtefact(std::string path, std::string& result) { - if(archiveUrl != ARCHIVE_URL_DEFAULT) { - // we shouldn't have got here. Probably an invalid link. - return false; +class MappedFileImpl : public MappedFile { +public: + MappedFileImpl(const char* path) : + fd(open(path, O_RDONLY)), + sz(0), + ptr(nullptr) + { + if(fd == -1) return; + struct stat st; + if(fstat(fd, &st) != 0) return; + sz = st.st_size; + ptr = mmap(nullptr, sz, PROT_READ, MAP_SHARED, fd, 0); + if(ptr == MAP_FAILED) + ptr = nullptr; } - fs::path file(fs::path(homeDir)/"archive"/path); - return slurp(file, result); + ~MappedFileImpl() override { + if(ptr) + munmap(ptr, sz); + if(fd != -1) + close(fd); + } + virtual const void* address() override { return ptr; } + virtual size_t size() override { return sz; } +private: + int fd; + size_t sz; + void* ptr; +}; + +kj::Own Laminar::getArtefact(std::string path) { + return kj::heap(fs::path(fs::path(homeDir)/"archive"/path).c_str()); } std::string Laminar::getCustomCss() diff --git a/src/laminar.h b/src/laminar.h index d452ad6..1f8efd3 100644 --- a/src/laminar.h +++ b/src/laminar.h @@ -55,7 +55,7 @@ public: void sendStatus(LaminarClient* client) override; bool setParam(std::string job, uint buildNum, std::string param, std::string value) override; - bool getArtefact(std::string path, std::string& result) override; + kj::Own getArtefact(std::string path) override; std::string getCustomCss() override; void abortAll() override; void reapChildren() override; diff --git a/src/server.cpp b/src/server.cpp index b1b352d..55fa849 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -311,19 +311,18 @@ private: const char* start, *end, *content_type; responseHeaders.clear(); if(resource.compare(0, strlen("/archive/"), "/archive/") == 0) { - std::string file(resource.substr(strlen("/archive/"))); - std::string content; - if(laminar.getArtefact(file, content)) { + kj::Own file = laminar.getArtefact(resource.substr(strlen("/archive/"))); + if(file->address() != nullptr) { responseHeaders.add("Content-Transfer-Encoding", "binary"); - auto stream = response.send(200, "OK", responseHeaders, content.size()); - return stream->write(content.data(), content.size()).attach(kj::mv(stream)); + auto stream = response.send(200, "OK", responseHeaders, file->size()); + return stream->write(file->address(), file->size()).attach(kj::mv(file)).attach(kj::mv(stream)); } } else if(resource.compare("/custom/style.css") == 0) { responseHeaders.set(kj::HttpHeaderId::CONTENT_TYPE, "text/css; charset=utf-8"); responseHeaders.add("Content-Transfer-Encoding", "binary"); std::string css = laminar.getCustomCss(); auto stream = response.send(200, "OK", responseHeaders, css.size()); - return stream->write(css.data(), css.size()).attach(kj::mv(stream)); + return stream->write(css.data(), css.size()).attach(kj::mv(css)).attach(kj::mv(stream)); } else if(resources.handleRequest(resource, &start, &end, &content_type)) { responseHeaders.set(kj::HttpHeaderId::CONTENT_TYPE, content_type); responseHeaders.add("Content-Encoding", "gzip"); diff --git a/test/test-server.cpp b/test/test-server.cpp index 27b4187..2fa1e21 100644 --- a/test/test-server.cpp +++ b/test/test-server.cpp @@ -44,6 +44,7 @@ public: class MockLaminar : public LaminarInterface { public: LaminarClient* client = nullptr; + ~MockLaminar() {} virtual void registerClient(LaminarClient* c) override { ASSERT_EQ(nullptr, client); client = c; @@ -55,12 +56,14 @@ public: client = nullptr; } + // MOCK_METHOD does not seem to work with return values whose destructors have noexcept(false) + kj::Own getArtefact(std::string path) override { return kj::Own(nullptr, kj::NullDisposer()); } + MOCK_METHOD2(queueJob, std::shared_ptr(std::string name, ParamMap params)); MOCK_METHOD1(registerWaiter, void(LaminarWaiter* waiter)); MOCK_METHOD1(deregisterWaiter, void(LaminarWaiter* waiter)); MOCK_METHOD1(sendStatus, void(LaminarClient* client)); MOCK_METHOD4(setParam, bool(std::string job, uint buildNum, std::string param, std::string value)); - MOCK_METHOD2(getArtefact, bool(std::string path, std::string& result)); MOCK_METHOD0(getCustomCss, std::string()); MOCK_METHOD0(abortAll, void()); MOCK_METHOD0(reapChildren, void());