From cb2af050043dbf086c42c5981ea47f2ee557c7e5 Mon Sep 17 00:00:00 2001
From: emeric <emeric.poupon@laposte.net>
Date: Fri, 2 Apr 2021 13:26:27 +0200
Subject: [PATCH] More robust child process handling

---
 src/libs/utils/impl/ChildProcess.cpp | 34 ++++++++++++----------------
 src/libs/utils/impl/ChildProcess.hpp |  4 +---
 2 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/src/libs/utils/impl/ChildProcess.cpp b/src/libs/utils/impl/ChildProcess.cpp
index c4eba387..82f319fc 100644
--- a/src/libs/utils/impl/ChildProcess.cpp
+++ b/src/libs/utils/impl/ChildProcess.cpp
@@ -46,6 +46,10 @@ namespace
 			SystemException(int err, const std::string& errMsg)
 				: ChildProcessException {errMsg + ": " + strerror(err)}
 			{}
+
+			SystemException(boost::system::error_code ec, const std::string& errMsg)
+				: ChildProcessException {errMsg + ": " + ec.message()}
+			{}
 	};
 }
 
@@ -103,7 +107,7 @@ ChildProcess::ChildProcess(boost::asio::io_context& ioContext, const std::filesy
 			boost::system::error_code assignError;
 			_childStdout.assign(pipe[0], assignError);
 			if (assignError)
-				LMS_LOG(CHILDPROCESS, ERROR) << "Assign failed: " << assignError.message();
+				throw SystemException {assignError, "fork failed!"};
 		}
 		_childPID = res;
 	}
@@ -113,6 +117,7 @@ ChildProcess::~ChildProcess()
 {
 	if (!_waited)
 	{
+		LMS_LOG(CHILDPROCESS, DEBUG) << "Closing child process...";
 		{
 			boost::system::error_code closeError;
 			_childStdout.close(closeError);
@@ -136,6 +141,7 @@ ChildProcess::drain()
 void
 ChildProcess::kill()
 {
+	LMS_LOG(CHILDPROCESS, DEBUG) << "Killing child process...";
 	::kill(_childPID, SIGKILL);
 }
 
@@ -162,36 +168,25 @@ ChildProcess::wait(bool block)
 void
 ChildProcess::asyncRead(std::byte* data, std::size_t bufferSize, ReadCallback callback) 
 {
+	assert(!finished());
+
 	boost::asio::async_read(_childStdout, boost::asio::buffer(data, bufferSize),
 		[this, callback {std::move(callback)}](const boost::system::error_code& error, std::size_t bytesTransferred)
 		{
 			LMS_LOG(CHILDPROCESS, DEBUG) << "Async read cb - ec = '" << error.message() << "', bytesTransferred = " << bytesTransferred;
 
+			ReadResult readResult {ReadResult::Success};
 			if (error)
 			{
-				if (error == boost::asio::error::operation_aborted)
-				{
-					return;
-				}
-
-				{
-					boost::system::error_code closeError;
-					_childStdout.close(closeError);
-				}
+				_finished = true;
 
 				if (error == boost::asio::error::eof)
-				{
-					callback(ReadResult::EndOfFile, bytesTransferred);
-					return;
-				}
+					readResult = ReadResult::EndOfFile;
 				else
-				{
-					callback(ReadResult::Error, bytesTransferred);
 					return;
-				}
 			}
 
-			callback(ReadResult::Success, bytesTransferred);
+			callback(readResult, bytesTransferred);
 		});
 }
 
@@ -199,6 +194,7 @@ void
 ChildProcess::asyncWaitForData(WaitCallback cb)
 {
 	LMS_LOG(CHILDPROCESS, DEBUG) << "Async wait requested";
+	assert(!finished());
 
 	_childStdout.async_wait(boost::asio::posix::stream_descriptor::wait_read,
 			[cb {std::move(cb)}](const boost::system::error_code& ec)
@@ -224,6 +220,6 @@ ChildProcess::readSome(std::byte* data, std::size_t bufferSize)
 bool
 ChildProcess::finished()
 {
-	return !_childStdout.is_open();
+	return _finished;
 }
 
diff --git a/src/libs/utils/impl/ChildProcess.hpp b/src/libs/utils/impl/ChildProcess.hpp
index e3c74f9a..f92b6b25 100644
--- a/src/libs/utils/impl/ChildProcess.hpp
+++ b/src/libs/utils/impl/ChildProcess.hpp
@@ -20,9 +20,6 @@
 #pragma once
 
 #include <cstddef>
-#include <functional>
-#pragma once
-
 #include <sys/types.h>
 #include <unistd.h>
 
@@ -56,5 +53,6 @@ class ChildProcess : public IChildProcess
 		FileDescriptor				_childStdout;
 		::pid_t						_childPID {};
 		bool						_waited {};
+		bool						_finished {};
 		std::optional<int>			_exitCode;
 };