From db65031c2b9f8df79c7626881f2b94aadbba7f9c Mon Sep 17 00:00:00 2001
From: Zygo Blaxell <bees@furryterror.org>
Date: Wed, 28 Apr 2021 21:10:55 -0400
Subject: [PATCH] context: get rid of all instances of pthread_cancel

pthread_cancel doesn't really work properly.  It was only being used in
bees to bring threads to a stop if the BeesContext is destroyed early.
It is frequently implicated in core dump reports because of the fragility
of the C++ iostream / C stdio / library infrastructure, particularly
surrounding upgrades on the host running bees.  The pthread_cancel call
itself often simply fails even when it doesn't call terminate().

Defer creation of the status and progress threads until after the
BeesContext::start method is invoked.  At that point, the existing
ask-threads-nicely-to-stop code is up and running, and normal condvars
can be used to bring bees to a stop, without having to resort to
pthread_cancel.

Since we're deleting half of the BeesContext constructor in this change,
let's remove the other half too, and put an end to the deprecated support
for multiple BeesContexts sharing a process.  It's still possible to run
multiple BeesContexts, but they will not share a FD cache.  This will
allow the FD cache's keys to become smaller and hopefully save some
memory later on.

Fixes: #171

Signed-off-by: Zygo Blaxell <bees@furryterror.org>
---
 src/bees-context.cc | 29 +++++++++++------------------
 src/bees-thread.cc  |  5 -----
 src/bees.h          |  6 +++---
 3 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/src/bees-context.cc b/src/bees-context.cc
index 24d8039..7d53893 100644
--- a/src/bees-context.cc
+++ b/src/bees-context.cc
@@ -178,22 +178,6 @@ BeesContext::home_fd()
 	return m_home_fd;
 }
 
-BeesContext::BeesContext(shared_ptr<BeesContext> parent) :
-	m_parent_ctx(parent),
-	m_progress_thread("progress_report"),
-	m_status_thread("status_report")
-{
-	if (m_parent_ctx) {
-		m_fd_cache = m_parent_ctx->fd_cache();
-	}
-	m_progress_thread.exec([=]() {
-		show_progress();
-	});
-	m_status_thread.exec([=]() {
-		dump_status();
-	});
-}
-
 bool
 BeesContext::is_root_ro(uint64_t root)
 {
@@ -962,6 +946,15 @@ BeesContext::start()
 	BEESLOGNOTICE("Starting bees main loop...");
 	BEESNOTE("starting BeesContext");
 
+	m_progress_thread = make_shared<BeesThread>("progress_report");
+	m_status_thread = make_shared<BeesThread>("status_report");
+	m_progress_thread->exec([=]() {
+		show_progress();
+	});
+	m_status_thread->exec([=]() {
+		dump_status();
+	});
+
 	// Set up temporary file pool
 	m_tmpfile_pool.generator([=]() -> shared_ptr<BeesTempFile> {
 		return make_shared<BeesTempFile>(shared_from_this());
@@ -1043,7 +1036,7 @@ BeesContext::stop()
 
 	BEESNOTE("waiting for progress thread");
 	BEESLOGDEBUG("Waiting for progress thread");
-	m_progress_thread.join();
+	m_progress_thread->join();
 
 	// XXX: nobody can see this BEESNOTE because we are killing the
 	// thread that publishes it
@@ -1053,7 +1046,7 @@ BeesContext::stop()
 	m_stop_status = true;
 	m_stop_condvar.notify_all();
 	lock.unlock();
-	m_status_thread.join();
+	m_status_thread->join();
 
 	BEESLOGNOTICE("bees stopped in " << stop_timer << " sec");
 }
diff --git a/src/bees-thread.cc b/src/bees-thread.cc
index 1808ad4..4445c46 100644
--- a/src/bees-thread.cc
+++ b/src/bees-thread.cc
@@ -70,11 +70,6 @@ BeesThread::~BeesThread()
 
 	BEESLOGDEBUG("BeesThread destructor " << m_name);
 	if (m_thread_ptr->joinable()) {
-		BEESLOGDEBUG("Cancelling thread " << m_name);
-		int rv = pthread_cancel(m_thread_ptr->native_handle());
-		if (rv) {
-			BEESLOGDEBUG("pthread_cancel returned " << strerror(-rv));
-		}
 		BEESLOGDEBUG("Waiting for thread " << m_name);
 		Timer thread_time;
 		m_thread_ptr->join();
diff --git a/src/bees.h b/src/bees.h
index eeb30a7..699c78c 100644
--- a/src/bees.h
+++ b/src/bees.h
@@ -751,8 +751,8 @@ class BeesContext : public enable_shared_from_this<BeesContext> {
 	condition_variable				m_abort_condvar;
 	bool						m_abort_requested = false;
 
-	BeesThread					m_progress_thread;
-	BeesThread					m_status_thread;
+	shared_ptr<BeesThread>				m_progress_thread;
+	shared_ptr<BeesThread>				m_status_thread;
 
 	void set_root_fd(Fd fd);
 
@@ -763,7 +763,7 @@ class BeesContext : public enable_shared_from_this<BeesContext> {
 	void rewrite_file_range(const BeesFileRange &bfr);
 
 public:
-	BeesContext(shared_ptr<BeesContext> parent_ctx = nullptr);
+	BeesContext() = default;
 
 	void set_root_path(string path);