From 1e8bb0de0b0b05dc1323520dfa57df1f19b51b83 Mon Sep 17 00:00:00 2001
From: crupest <crupest@outlook.com>
Date: Mon, 24 Jun 2024 00:06:25 +0800
Subject: fix: a lock bug and add test for Wait.

NEED TEST: BufferStream, AutoReadStream, SubProcess.
---
 include/cru/common/SubProcess.h | 10 +++++++++-
 src/common/SubProcess.cpp       | 28 +++++++++++++++++++---------
 test/common/SubProcessTest.cpp  |  2 ++
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/include/cru/common/SubProcess.h b/include/cru/common/SubProcess.h
index 078d5546..98c272a3 100644
--- a/include/cru/common/SubProcess.h
+++ b/include/cru/common/SubProcess.h
@@ -65,6 +65,10 @@ struct SubProcessExitResult {
   int exit_signal;
   bool has_core_dump;
 
+  bool IsSuccess() const {
+    return exit_type == SubProcessExitType::Normal && exit_code == 0;
+  }
+
   static SubProcessExitResult Unknown() {
     return {SubProcessExitType::Unknown, 0, 0, false};
   }
@@ -139,7 +143,6 @@ class PlatformSubProcess : public Object {
         : start_info(std::move(start_info)), impl(std::move(impl)) {}
 
     std::mutex mutex;
-    std::unique_lock<std::mutex> lock{mutex, std::defer_lock};
     std::condition_variable condition_variable;
     SubProcessStartInfo start_info;
     SubProcessExitResult exit_result;
@@ -205,6 +208,7 @@ class PlatformSubProcess : public Object {
 
  private:
   std::shared_ptr<State> state_;
+  std::unique_lock<std::mutex> lock_;
 };
 
 class CRU_BASE_API SubProcess : public Object {
@@ -215,6 +219,10 @@ class CRU_BASE_API SubProcess : public Object {
       String program, std::vector<String> arguments = {},
       std::unordered_map<String, String> environments = {});
 
+  static SubProcessExitResult Call(
+      String program, std::vector<String> arguments = {},
+      std::unordered_map<String, String> environments = {});
+
  public:
   SubProcess(SubProcessStartInfo start_info);
 
diff --git a/src/common/SubProcess.cpp b/src/common/SubProcess.cpp
index 0ffb2387..33926f39 100644
--- a/src/common/SubProcess.cpp
+++ b/src/common/SubProcess.cpp
@@ -15,12 +15,13 @@ using ThisPlatformSubProcessImpl = platform::unix::PosixSpawnSubProcessImpl;
 PlatformSubProcess::PlatformSubProcess(
     SubProcessStartInfo start_info,
     std::shared_ptr<IPlatformSubProcessImpl> impl)
-    : state_(new State(std::move(start_info), std::move(impl))) {}
+    : state_(new State(std::move(start_info), std::move(impl))),
+      lock_(state_->mutex, std::defer_lock) {}
 
 PlatformSubProcess::~PlatformSubProcess() {}
 
 void PlatformSubProcess::Start() {
-  std::lock_guard lock_guard(this->state_->lock);
+  std::lock_guard lock_guard(this->lock_);
 
   if (this->state_->status != SubProcessStatus::Prepare) {
     throw SubProcessException(u"The process has already tried to start.");
@@ -31,7 +32,7 @@ void PlatformSubProcess::Start() {
     this->state_->status = SubProcessStatus::Running;
 
     auto thread = std::thread([state = state_] {
-      std::lock_guard lock_guard(state->lock);
+      std::unique_lock lock(state->mutex);
       state->exit_result = state->impl->PlatformWaitForProcess();
       state->status = SubProcessStatus::Exited;
       state->condition_variable.notify_all();
@@ -47,7 +48,7 @@ void PlatformSubProcess::Start() {
 
 void PlatformSubProcess::Wait(
     std::optional<std::chrono::milliseconds> wait_time) {
-  std::lock_guard lock_guard(this->state_->lock);
+  std::lock_guard lock_guard(this->lock_);
 
   if (this->state_->status == SubProcessStatus::Prepare) {
     throw SubProcessException(
@@ -68,15 +69,15 @@ void PlatformSubProcess::Wait(
   };
 
   if (wait_time) {
-    this->state_->condition_variable.wait_for(this->state_->lock, *wait_time,
+    this->state_->condition_variable.wait_for(this->lock_, *wait_time,
                                               predicate);
   } else {
-    this->state_->condition_variable.wait(this->state_->lock, predicate);
+    this->state_->condition_variable.wait(this->lock_, predicate);
   }
 }
 
 void PlatformSubProcess::Kill() {
-  std::lock_guard lock_guard(this->state_->lock);
+  std::lock_guard lock_guard(this->lock_);
 
   if (this->state_->status == SubProcessStatus::Prepare) {
     throw SubProcessException(u"The process does not start. Can't kill it.");
@@ -99,12 +100,12 @@ void PlatformSubProcess::Kill() {
 }
 
 SubProcessStatus PlatformSubProcess::GetStatus() {
-  std::lock_guard lock_guard(this->state_->lock);
+  std::lock_guard lock_guard(this->lock_);
   return this->state_->status;
 }
 
 SubProcessExitResult PlatformSubProcess::GetExitResult() {
-  std::lock_guard lock_guard(this->state_->lock);
+  std::lock_guard lock_guard(this->lock_);
 
   if (this->state_->status == SubProcessStatus::Prepare) {
     throw SubProcessException(
@@ -145,6 +146,15 @@ SubProcess SubProcess::Create(String program, std::vector<String> arguments,
   return SubProcess(std::move(start_info));
 }
 
+SubProcessExitResult SubProcess::Call(
+    String program, std::vector<String> arguments,
+    std::unordered_map<String, String> environments) {
+  auto process =
+      Create(std::move(program), std::move(arguments), std::move(environments));
+  process.Wait();
+  return process.GetExitResult();
+}
+
 SubProcess::SubProcess(SubProcessStartInfo start_info) {
   platform_process_.reset(new PlatformSubProcess(
       std::move(start_info), std::make_shared<ThisPlatformSubProcessImpl>()));
diff --git a/test/common/SubProcessTest.cpp b/test/common/SubProcessTest.cpp
index f2df2142..e42ee943 100644
--- a/test/common/SubProcessTest.cpp
+++ b/test/common/SubProcessTest.cpp
@@ -7,6 +7,8 @@ using cru::SubProcess;
 TEST_CASE("SubProcess", "[subprocess]") {
   SECTION("should work.") {
     SubProcess process = SubProcess::Create(u"echo", {u"abc"});
+    process.Wait();
+    REQUIRE(process.GetExitResult().IsSuccess());
     auto output = process.GetStdoutStream()->ReadToEndAsUtf8String();
     REQUIRE(output == u"abc\n");
   }
-- 
cgit v1.2.3