Fix race between flush error recovery and db destruction

[hx235]

Context:
DB destruction will wait for ongoing error recovery through EndAutoRecovery() and join the recovery thread:

rocksdb/db/db_impl/db_impl.cc

Line 525 in 519f2a4

error_handler_.CancelErrorRecovery();

->

rocksdb/db/error_handler.cc

Line 250 in 519f2a4

EndAutoRecovery();

->

rocksdb/db/error_handler.cc

Lines 808 to 823 in 519f2a4

	void ErrorHandler::EndAutoRecovery() {
	db_mutex_->AssertHeld();
	if (!end_recovery_) {
	end_recovery_ = true;
	}
	if (recovery_thread_) {
	// Ensure only one thread can execute the join().
	std::unique_ptr<port::Thread> old_recovery_thread(
	std::move(recovery_thread_));
	db_mutex_->Unlock();
	cv_.SignalAll();
	old_recovery_thread->join();
	db_mutex_->Lock();
	}
	return;
	}

However, due to a race between flush error recovery and db destruction, recovery can actually start after such wait during the db shutdown. The consequence is that the recovery thread created as part of this recovery will not be properly joined upon its destruction as part the db destruction. It then crashes the program as below.

std::terminate() 
std::default_delete<std::thread>::operator()(std::thread*) const 
std::unique_ptr<std::thread, std::default_delete<std::thread>>::~unique_ptr()
rocksdb::ErrorHandler::~ErrorHandler() (rocksdb/db/error_handler.h:31)
rocksdb::DBImpl::~DBImpl() (rocksdb/db/db_impl/db_impl.cc:725)
rocksdb::DBImpl::~DBImpl() (rocksdb/db/db_impl/db_impl.cc:725)
rocksdb::DBTestBase::Close() (rocksdb/db/db_test_util.cc:678)

Summary:
This PR fixed it by considering whether EndAutoRecovery() has been called before creating such thread. This fix is similar to how we currently handle such case inside the created recovery thread.

Test plan:
A new UT repro-ed the crash before this fix and and pass after.

[facebook-github-bot]

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@hx235 has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ajkr]

LGTM. We should see if there's some patterns in the recent ErrorHandler race conditions. #11955 #11950 #11939 #11937 #11890 #11880 #11991. Personally I think it has too much state and maybe could be implemented as functions on DBImpl

[facebook-github-bot]

@hx235 has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@hx235 merged this pull request in 0f14135.