linux: Fix and simplify stdout/stderr redirection

Use close() instead of some dodgy read unblocking and thread cancelling
sequence - it fixes a race condition when closing PCSX2.

Move most of the setup/cleanup into LinuxPipeThread. In addition to
simplifying things, it should also mean that no messages to
stdout/stderr are lost - in the previous code there was a small period
of time where messages would disappear.

pcsx2/Linux/LnxConsolePipe.cpp

@@ -21,47 +21,100 @@
 
 using namespace Threading;
 
-// Reads data sent to stdout/stderr and sends it to the PCSX2 console.
+// Redirects and reads data sent to stdout/stderr and sends it to the PCSX2
+// console.
 class LinuxPipeThread : public pxThread
 {
 	typedef pxThread _parent;
 
 protected:
-	const int& m_read_fd;
+	FILE* m_stdstream;
+	FILE* m_fp;
 	const ConsoleColors m_color;
+	int m_pipe_fd[2];
 
 	void ExecuteTaskInThread();
 public:
-	LinuxPipeThread(const int& read_fd, ConsoleColors color);
+	LinuxPipeThread(FILE* stdstream);
 	virtual ~LinuxPipeThread() noexcept;
 };
 
-LinuxPipeThread::LinuxPipeThread(const int& read_fd, ConsoleColors color)
+LinuxPipeThread::LinuxPipeThread(FILE* stdstream)
-	: pxThread(color == Color_Red ? L"Redirect_Stderr" : L"Redirect_Stdout")
+	: pxThread(stdstream == stderr? L"Redirect_Stderr" : L"Redirect_Stdout")
-	, m_read_fd(read_fd)
+	, m_stdstream(stdstream)
-	, m_color(color)
+	, m_fp(nullptr)
+	, m_color(stdstream == stderr? Color_Red : Color_Black)
+	, m_pipe_fd{-1, -1}
 {
+	const wxChar* stream_name = stdstream == stderr? L"stderr" : L"stdout";
+
+	// Save the original stdout/stderr file descriptor
+	int dup_fd = dup(fileno(stdstream));
+	if (dup_fd == -1)
+		throw Exception::RuntimeError().SetDiagMsg(wxString::Format(
+			L"Redirect %s failed: dup: %s", stream_name, strerror(errno)));
+	m_fp = fdopen(dup_fd, "w");
+	if (m_fp == nullptr) {
+		int error = errno;
+		close(dup_fd);
+		throw Exception::RuntimeError().SetDiagMsg(wxString::Format(
+			L"Redirect %s failed: fdopen: %s", stream_name, strerror(error)));
+	}
+
+	if (pipe(m_pipe_fd) == -1) {
+		int error = errno;
+		fclose(m_fp);
+		throw Exception::RuntimeError().SetDiagMsg(wxString::Format(
+			L"Redirect %s failed: pipe: %s", stream_name, strerror(error)));
+	}
 }
 
 LinuxPipeThread::~LinuxPipeThread() noexcept
 {
-	_parent::Cancel();
+	// Close write end of the pipe first so the redirection thread starts
+	// finishing up and restore the original stdout/stderr file descriptor so
+	// messages aren't lost.
+	dup2(fileno(m_fp), m_pipe_fd[1]);
+
+	if (m_stdstream == stdout)
+		Console_SetStdout(stdout);
+	fclose(m_fp);
+
+	// Read end of pipe should only be closed after the thread terminates to
+	// prevent messages being lost.
+	m_mtx_InThread.Wait();
+	close(m_pipe_fd[0]);
 }
 
 void LinuxPipeThread::ExecuteTaskInThread()
 {
-	char buffer[2049];
+	const wxChar* stream_name = m_stdstream == stderr? L"stderr" : L"stdout";
-	while (ssize_t bytes_read = read(m_read_fd, buffer, sizeof(buffer) - 1)) {
-		TestCancel();
 
+	// Redirect stdout/stderr
+	int stdstream_fd = fileno(m_stdstream);
+	if (dup2(m_pipe_fd[1], stdstream_fd) != stdstream_fd) {
+		Console.Error(wxString::Format(L"Redirect %s failed: dup2: %s",
+			stream_name, strerror(errno)));
+		return;
+	}
+
+	close(m_pipe_fd[1]);
+	m_pipe_fd[1] = stdstream_fd;
+
+	// Send console output to the original stdout, otherwise there'll be an
+	// infinite loop.
+	if (m_stdstream == stdout)
+		Console_SetStdout(m_fp);
+
+	char buffer[2049];
+	while (ssize_t bytes_read = read(m_pipe_fd[0], buffer, sizeof(buffer) - 1)) {
 		if (bytes_read == -1) {
 			if (errno == EINTR) {
 				continue;
-			}
+			} else {
-			else {
 				// Should never happen.
 				Console.Error(wxString::Format(L"Redirect %s failed: read: %s",
-					m_color == Color_Red ? L"stderr" : L"stdout", strerror(errno)));
+					stream_name, strerror(errno)));
 				break;
 			}
 		}
@@ -72,111 +125,37 @@ void LinuxPipeThread::ExecuteTaskInThread()
 			ConsoleColorScope cs(m_color);
 			Console.WriteRaw(fromUTF8(buffer));
 		}
-
-		TestCancel();
 	}
 }
 
-// Redirects data sent to stdout/stderr into a pipe, and sets up a thread to
-// forward data to the console.
 class LinuxPipeRedirection : public PipeRedirectionBase
 {
 	DeclareNoncopyableObject(LinuxPipeRedirection);
 
 protected:
-	FILE* m_stdstream;
-	FILE* m_fp;
-	int m_pipe_fd[2];
 	LinuxPipeThread m_thread;
 
-	void Cleanup() noexcept;
 public:
 	LinuxPipeRedirection(FILE* stdstream);
 	virtual ~LinuxPipeRedirection() noexcept;
 };
 
 LinuxPipeRedirection::LinuxPipeRedirection(FILE* stdstream)
-	: m_stdstream(stdstream)
+	: m_thread(stdstream)
-	, m_fp(nullptr)
-	, m_pipe_fd{-1, -1}
-	, m_thread(m_pipe_fd[0], stdstream == stderr ? Color_Red : Color_Black)
 {
 	pxAssert((stdstream == stderr) || (stdstream == stdout));
 
-	const wxChar* stream_name = stdstream == stderr? L"stderr" : L"stdout";
-
 	try {
-		int stdstream_fd = fileno(stdstream);
-
-		// Save the original stdout/stderr file descriptor...
-		int dup_fd = dup(stdstream_fd);
-		if (dup_fd == -1)
-			throw Exception::RuntimeError().SetDiagMsg(wxString::Format(
-				L"Redirect %s failed: dup: %s", stream_name, strerror(errno)));
-		m_fp = fdopen(dup_fd, "w");
-		if (m_fp == nullptr)
-			throw Exception::RuntimeError().SetDiagMsg(wxString::Format(
-				L"Redirect %s failed: fdopen: %s", stream_name, strerror(errno)));
-
-		// and now redirect stdout/stderr.
-		if (pipe(m_pipe_fd) == -1)
-			throw Exception::RuntimeError().SetDiagMsg(wxString::Format(
-				L"Redirect %s failed: pipe: %s", stream_name, strerror(errno)));
-		if (dup2(m_pipe_fd[1], stdstream_fd) != stdstream_fd)
-			throw Exception::RuntimeError().SetDiagMsg(wxString::Format(
-				L"Redirect %s failed: dup2: %s", stream_name, strerror(errno)));
-		close(m_pipe_fd[1]);
-		m_pipe_fd[1] = stdstream_fd;
-
-		// And send the final console output goes to the original stdout,
-		// otherwise we'll have an infinite data loop.
-		if (stdstream == stdout)
-			Console_SetStdout(m_fp);
-
 		m_thread.Start();
 	} catch (Exception::BaseThreadError& ex) {
 		// thread object will become invalid because of scoping after we leave
 		// the constructor, so re-pack a new exception:
-		Cleanup();
 		throw Exception::RuntimeError().SetDiagMsg(ex.FormatDiagnosticMessage());
-	} catch (...) {
-		Cleanup();
-		throw;
 	}
 }
 
 LinuxPipeRedirection::~LinuxPipeRedirection() noexcept
 {
-	Cleanup();
-}
-
-void LinuxPipeRedirection::Cleanup() noexcept
-{
-	// Restore stdout/stderr file descriptor - mostly useful if the thread
-	// fails to start, but then you have bigger issues to worry about.
-	if (m_pipe_fd[1] != -1) {
-		if (m_pipe_fd[1] == fileno(m_stdstream)) {
-			// FIXME: Use lock for better termination.
-			// The redirect thread is most likely waiting at read(). Send a
-			// newline so it returns and the thread begins to terminate.
-			fflush(m_stdstream);
-			m_thread.Cancel();
-			fputc('\n', m_stdstream);
-			fflush(m_stdstream);
-			dup2(fileno(m_fp), fileno(m_stdstream));
-		} else {
-			close(m_pipe_fd[1]);
-		}
-	}
-
-	if (m_fp != nullptr) {
-		if (m_stdstream == stdout)
-			Console_SetStdout(stdout);
-		fclose(m_fp);
-	}
-
-	if (m_pipe_fd[0] != -1)
-		close(m_pipe_fd[0]);
 }
 
 PipeRedirectionBase* NewPipeRedir(FILE* stdstream)