Refactor: simplify OS's execution scheme (#126)

Removed unnecessary ExecutionInfo and put everything in ExecutionResult instead. ExecutionResult now only has a string representing stderr instead of two istream*s. Caller should open the output/error files explicitly to read them, and then close them. This is to make it clear that the caller is responsible for closing the open files.
ia-toki · Mar 3, 2017 · 7dfb58c · 7dfb58c
1 parent c304556
commit 7dfb58c
Show file tree

Hide file tree

Showing 19 changed files with 129 additions and 225 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -72,7 +72,6 @@ set(INCLUDE
         include/tcframe/logger/LoggerEngine.hpp
         include/tcframe/logger/SimpleLoggerEngine.hpp
         include/tcframe/os.hpp
-        include/tcframe/os/ExecutionInfo.hpp
         include/tcframe/os/ExecutionResult.hpp
         include/tcframe/os/ExecutionRequest.hpp
         include/tcframe/os/OperatingSystem.hpp

diff --git a/include/tcframe/evaluator/BatchEvaluator.hpp b/include/tcframe/evaluator/BatchEvaluator.hpp
@@ -27,8 +27,7 @@ class BatchEvaluator : public Evaluator {
         ExecutionRequestBuilder request = ExecutionRequestBuilder()
                 .setCommand(config.solutionCommand())
                 .setInputFilename(inputFilename)
-                .setOutputFilename(outputFilename)
-                .setErrorFilename("_error.out");
+                .setOutputFilename(outputFilename);
 
         if (config.timeLimit()) {
             request.setTimeLimit(config.timeLimit().value());
@@ -46,9 +45,9 @@ class BatchEvaluator : public Evaluator {
         EvaluationResultBuilder result;
         result.setExecutionResult(executionResult);
 
-        if (executionResult.info().exceededCpuLimits()) {
+        if (executionResult.exceededCpuLimits()) {
             result.setVerdict(Verdict::tle());
-        } else if (!executionResult.info().isSuccessful()) {
+        } else if (!executionResult.isSuccessful()) {
             result.setVerdict(Verdict::rte());
         }
 

diff --git a/include/tcframe/generator/TestCaseGenerator.hpp b/include/tcframe/generator/TestCaseGenerator.hpp
@@ -105,7 +105,7 @@ class TestCaseGenerator {
         } else {
             ioManipulator_->printInput(input);
         }
-        os_->closeOpenedWritingStream(input);
+        os_->closeOpenedStream(input);
     }
 
     void evaluateAndApplyOutput(
@@ -128,17 +128,18 @@ class TestCaseGenerator {
 
         EvaluationResult evaluationResult = evaluator_->evaluate(inputFilename, outputFilename, evaluatorConfig);
         ExecutionResult executionResult = evaluationResult.executionResult();
-        if (!executionResult.info().isSuccessful()) {
+        if (!executionResult.isSuccessful()) {
             throw GenerationException([=] { logger_->logExecutionFailure("solution", executionResult); });
         }
 
         if (maybeSampleOutputString) {
             checkSampleOutput(maybeSampleOutputString.value(), inputFilename, outputFilename, config);
         }
 
-        istream* output = executionResult.outputStream();
+        istream* output = os_->openForReading(outputFilename);
         modifyOutputForMultipleTestCases(output, config);
         ioManipulator_->parseOutput(output);
+        os_->closeOpenedStream(output);
     }
 
     optional<string> getSampleOutputString(const TestCase& testCase) {
@@ -186,13 +187,13 @@ class TestCaseGenerator {
 
         ostream* sampleOutput = os_->openForWriting("_evaluation.out");
         *sampleOutput << modifiedSampleOutputString;
-        os_->closeOpenedWritingStream(sampleOutput);
+        os_->closeOpenedStream(sampleOutput);
 
         ScoringResult scoringResult = scorer_->score(inputFilename, outputFilename, "_evaluation.out");
         if (!(scoringResult.verdict() == Verdict::ac())) {
             throw GenerationException([=] {
                 logger_->logSampleTestCaseCheckFailure(scoringResult.message());
-                if (scoringResult.executionResult().info().isSuccessful()) {
+                if (scoringResult.executionResult().isSuccessful()) {
                     logger_->logTestCaseScoringMessage(scoringResult.message());
                 } else {
                     logger_->logExecutionFailure("scorer", scoringResult.executionResult());

diff --git a/include/tcframe/grader/TestCaseGrader.hpp b/include/tcframe/grader/TestCaseGrader.hpp
@@ -67,7 +67,7 @@ class TestCaseGrader {
 
         logger_->logTestCaseVerdict(result.verdict());
         if (!(result.verdict() == Verdict::ac())) {
-            if (result.executionResult().info().isSuccessful()) {
+            if (result.executionResult().isSuccessful()) {
                 logger_->logTestCaseScoringMessage(result.message());
             } else {
                 logger_->logExecutionFailure("scorer", result.executionResult());

diff --git a/include/tcframe/logger/BaseLogger.hpp b/include/tcframe/logger/BaseLogger.hpp
@@ -38,12 +38,11 @@ class BaseLogger {
 
     virtual void logExecutionFailure(const string& context, const ExecutionResult& result) {
         engine_->logListItem1(2, "Execution of " + context + " failed:");
-        if (result.info().exitCode()) {
-            engine_->logListItem2(3, "Exit code: " + StringUtils::toString(result.info().exitCode().value()));
-            engine_->logListItem2(3, "Standard error: " + string(
-                    StringUtils::streamToString(result.errorStream())));
+        if (result.exitCode()) {
+            engine_->logListItem2(3, "Exit code: " + StringUtils::toString(result.exitCode().value()));
+            engine_->logListItem2(3, "Standard error: " + result.standardError());
         } else {
-            engine_->logListItem2(3, "Exit signal: " + result.info().exitSignal().value());
+            engine_->logListItem2(3, "Exit signal: " + result.exitSignal().value());
         }
     }
 };

diff --git a/include/tcframe/os.hpp b/include/tcframe/os.hpp
@@ -1,6 +1,5 @@
 #pragma once
 
-#include "tcframe/os/ExecutionInfo.hpp"
 #include "tcframe/os/ExecutionResult.hpp"
 #include "tcframe/os/ExecutionRequest.hpp"
 #include "tcframe/os/OperatingSystem.hpp"

diff --git a/include/tcframe/os/ExecutionInfo.hpp b/include/tcframe/os/ExecutionInfo.hpp
diff --git a/include/tcframe/os/ExecutionResult.hpp b/include/tcframe/os/ExecutionResult.hpp
@@ -1,15 +1,13 @@
 #pragma once
 
-#include <istream>
-#include <sstream>
+#include <string>
 #include <tuple>
 #include <utility>
 
-#include "ExecutionInfo.hpp"
+#include "tcframe/util.hpp"
 
 using std::move;
-using std::istream;
-using std::istringstream;
+using std::string;
 using std::tie;
 
 namespace tcframe {
@@ -18,25 +16,35 @@ struct ExecutionResult {
     friend class ExecutionResultBuilder;
 
 private:
-    ExecutionInfo info_;
-    istream* outputStream_;
-    istream* errorStream_;
+    optional<int> exitCode_;
+    optional<string> exitSignal_;
+    bool exceededCpuLimits_;
+    string standardError_;
 
 public:
-    ExecutionInfo info() const {
-        return info_;
+    const optional<int>& exitCode() const {
+        return exitCode_;
     }
 
-    istream* outputStream() const {
-        return outputStream_;
+    const optional<string>& exitSignal() const {
+        return exitSignal_;
     }
 
-    istream* errorStream() const {
-        return errorStream_;
+    bool exceededCpuLimits() const {
+        return exceededCpuLimits_;
+    }
+
+    const string standardError() const {
+        return standardError_;
+    }
+
+    bool isSuccessful() const {
+        return exitCode_ && exitCode_.value() == 0;
     }
 
     bool operator==(const ExecutionResult& o) const {
-        return tie(info_) == tie(o.info_);
+        return tie(exitCode_, exitSignal_, exceededCpuLimits_, standardError_)
+               == tie(o.exitCode_, o.exitSignal_, o.exceededCpuLimits_, standardError_);
     }
 };
 
@@ -45,28 +53,31 @@ class ExecutionResultBuilder {
     ExecutionResult subject_;
 
 public:
-    ExecutionResultBuilder& setInfo(ExecutionInfo info) {
-        subject_.info_ = info;
+    ExecutionResultBuilder() {
+        subject_.exceededCpuLimits_ = false;
+    }
+
+    ExecutionResultBuilder& setExitCode(int exitCode) {
+        subject_.exitCode_ = optional<int>(exitCode);
+        return *this;
+    }
+
+    ExecutionResultBuilder& setExitSignal(string exitSignal) {
+        subject_.exitSignal_ = optional<string>(exitSignal);
         return *this;
     }
 
-    ExecutionResultBuilder& setOutputStream(istream* outputStream) {
-        subject_.outputStream_ = outputStream;
+    ExecutionResultBuilder& setExceededCpuLimits(bool exceededCpuLimits) {
+        subject_.exceededCpuLimits_ = exceededCpuLimits;
         return *this;
     }
 
-    ExecutionResultBuilder& setErrorStream(istream* errorStream) {
-        subject_.errorStream_ = errorStream;
+    ExecutionResultBuilder& setStandardError(string standardError) {
+        subject_.standardError_ = standardError;
         return *this;
     }
 
     ExecutionResult build() {
-        if (subject_.errorStream_ == nullptr) {
-            subject_.errorStream_ = new istringstream();
-        }
-        if (subject_.outputStream_ == nullptr) {
-            subject_.outputStream_ = new istringstream();
-        }
         return move(subject_);
     }
 };

diff --git a/include/tcframe/os/OperatingSystem.hpp b/include/tcframe/os/OperatingSystem.hpp
@@ -7,6 +7,7 @@
 #include "ExecutionRequest.hpp"
 #include "ExecutionResult.hpp"
 
+using std::ios;
 using std::istream;
 using std::ostream;
 using std::string;
@@ -19,7 +20,7 @@ class OperatingSystem {
 
     virtual istream* openForReading(const string& filename) = 0;
     virtual ostream* openForWriting(const string& filename) = 0;
-    virtual void closeOpenedWritingStream(ostream* out) = 0;
+    virtual void closeOpenedStream(ios* stream) = 0;
     virtual void forceMakeDir(const string& dirName) = 0;
     virtual void removeFile(const string& filename) = 0;
     virtual ExecutionResult execute(const ExecutionRequest& request) = 0;