Only start watching config in prepared state.

To catch config file changes that happened before CreateBuffers(), we unconditionally check the file when the watcher starts. This is likely more efficient and makes the code much simpler since there is no need to deal with the watcher racing with the PreparedState construction or destruction.
dechamps · Sep 4, 2020 · ecabb9a · ecabb9a
1 parent 85e3128
commit ecabb9a
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 59 deletions.
diff --git a/src/flexasio/FlexASIO/config.cpp b/src/flexasio/FlexASIO/config.cpp
@@ -123,20 +123,24 @@ namespace flexasio {
 
 	}
 
-	void ConfigLoader::HandleCloser::operator()(HANDLE handle) {
+	void ConfigLoader::Watcher::HandleCloser::operator()(HANDLE handle) {
 		if (::CloseHandle(handle) == 0)
 			throw std::system_error(::GetLastError(), std::system_category(), "unable to close handle");
 	}
 
-	ConfigLoader::Watcher::Watcher(std::function<void()> onConfigFileEvent, const std::filesystem::path& configDirectory) : onConfigFileEvent(std::move(onConfigFileEvent)), stopEvent([&] {
+	ConfigLoader::Watcher::Watcher(const ConfigLoader& configLoader, std::function<void()> onConfigChange) :
+		configLoader(configLoader),
+		onConfigChange(std::move(onConfigChange)),
+		stopEvent([&] {
 		const auto handle = CreateEventA(NULL, TRUE, FALSE, NULL);
 		if (handle == NULL)
 			throw std::system_error(::GetLastError(), std::system_category(), "Unable to create stop event");
 		return UniqueHandle(handle);
-		}()), directory([&] {
+		}()),
+		directory([&] {
 		Log() << "Opening config directory for watching";
 		const auto handle = ::CreateFileW(
-			configDirectory.wstring().c_str(),
+			configLoader.configDirectory.wstring().c_str(),
 			FILE_LIST_DIRECTORY,
 			FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
 			/*lpSecurityAttributes=*/NULL,
@@ -149,6 +153,7 @@ namespace flexasio {
 		}()) {
 		Log() << "Starting configuration file watcher";
 		StartWatching();
+		OnConfigFileEvent();
 		thread = std::thread([this] { RunThread(); });
 	}
 
@@ -197,7 +202,7 @@ namespace flexasio {
 
 		if (configFileEvent) {
 			Debounce();
-			onConfigFileEvent();
+			OnConfigFileEvent();
 		}
 
 		StartWatching();
@@ -279,23 +284,22 @@ namespace flexasio {
 			throw std::system_error(::GetLastError(), std::system_category(), "Unable to watch for directory changes");
 	}
 
-	ConfigLoader::ConfigLoader(std::function<void()> onConfigChange) :
-		onConfigChange(std::move(onConfigChange)),
+	ConfigLoader::ConfigLoader() :
 		configDirectory(GetUserDirectory()),
 		initialConfig(LoadConfig(configDirectory / configFileName)) {}
 
-	void ConfigLoader::OnConfigFileEvent() {
+	void ConfigLoader::Watcher::OnConfigFileEvent() {
 		Log() << "Handling config file event";
 
 		Config newConfig;
 		try {
-			newConfig = LoadConfig(configDirectory / configFileName);
+			newConfig = LoadConfig(configLoader.configDirectory / configFileName);
 		}
 		catch (const std::exception& exception) {
 			Log() << "Unable to load config, ignoring event: " << ::dechamps_cpputil::GetNestedExceptionMessage(exception);
 			return;
 		}
-		if (newConfig == initialConfig) {
+		if (newConfig == configLoader.Initial()) {
 			Log() << "New config is identical to initial config, not taking any action";
 			return;
 		}

diff --git a/src/flexasio/FlexASIO/config.h b/src/flexasio/FlexASIO/config.h
@@ -47,24 +47,21 @@ namespace flexasio {
 
 	class ConfigLoader {
 	public:
-		ConfigLoader(std::function<void()> onConfigChange);
+		ConfigLoader();
 
 		const Config& Initial() const { return initialConfig; }
 
-	private:
-		void OnConfigFileEvent();
-
-		struct HandleCloser {
-			void operator()(HANDLE handle);
-		};
-		using UniqueHandle = std::unique_ptr<std::remove_pointer_t<HANDLE>, HandleCloser>;
-
 		class Watcher {
 		public:
-			Watcher(std::function<void()> onConfigFileEvent, const std::filesystem::path& configDirectory);
+			Watcher(const ConfigLoader& configLoader, std::function<void()> onConfigChange);
 			~Watcher() noexcept(false);
 
 		private:
+			struct HandleCloser {
+				void operator()(HANDLE handle);
+			};
+			using UniqueHandle = std::unique_ptr<std::remove_pointer_t<HANDLE>, HandleCloser>;
+
 			struct OverlappedWithEvent {
 				OverlappedWithEvent();
 				~OverlappedWithEvent();
@@ -78,18 +75,19 @@ namespace flexasio {
 			void Debounce();
 			bool FillNotifyInformationBuffer();
 			bool FindConfigFileEvents();
+			void OnConfigFileEvent();
 
-			const std::function<void()> onConfigFileEvent;
+			const ConfigLoader& configLoader;
+			const std::function<void()> onConfigChange;
 			const UniqueHandle stopEvent;
 			const UniqueHandle directory;
 			OverlappedWithEvent overlapped;
 			alignas(DWORD) char fileNotifyInformationBuffer[64 * 1024];
 			std::thread thread;
 		};
 
-		const std::function<void()> onConfigChange;
+	private:
 		const std::filesystem::path configDirectory;
-		const Watcher watcher{ [this] { OnConfigFileEvent(); }, configDirectory };
 		const Config initialConfig;
 	};
 

diff --git a/src/flexasio/FlexASIO/flexasio.cpp b/src/flexasio/FlexASIO/flexasio.cpp
@@ -658,22 +658,7 @@ namespace flexasio {
 			// See https://github.com/dechamps/FlexASIO/issues/31
 			Log() << "WARNING: ASIO host application never enquired about sample rate, and therefore cannot know we are running at " << sampleRate << " Hz!";
 		}
-		bool resetRequested;
-		{
-			const std::lock_guard resetRequestLock(resetRequestMutex);
-			preparedState.emplace(*this, sampleRate, bufferInfos, numChannels, bufferSize, callbacks);
-			resetRequested = this->resetRequested;
-			this->resetRequested = false;
-		}
-		if (resetRequested) {
-			Log() << "Acting on a previous reset request";
-			try {
-				preparedState->RequestReset();
-			}
-			catch (const std::exception& exception) {
-				Log() << "Reset request failed: " << ::dechamps_cpputil::GetNestedExceptionMessage(exception);
-			}
-		}
+		preparedState.emplace(*this, sampleRate, bufferInfos, numChannels, bufferSize, callbacks);
 	}
 
 	FlexASIO::PreparedState::Buffers::Buffers(size_t bufferSetCount, size_t inputChannelCount, size_t outputChannelCount, size_t bufferSizeInFrames, size_t inputSampleSizeInBytes, size_t outputSampleSizeInBytes) :
@@ -731,7 +716,8 @@ namespace flexasio {
 			bufferInfos.push_back(asioBufferInfo);
 		}
 		return bufferInfos;
-	}()), openStreamResult(flexASIO.OpenStream(buffers.inputChannelCount > 0, buffers.outputChannelCount > 0, sampleRate, unsigned long(bufferSizeInFrames), &PreparedState::StreamCallback, this)) {
+	}()), openStreamResult(flexASIO.OpenStream(buffers.inputChannelCount > 0, buffers.outputChannelCount > 0, sampleRate, unsigned long(bufferSizeInFrames), &PreparedState::StreamCallback, this)),
+		configWatcher(flexASIO.configLoader, [this] { OnConfigChange(); }) {
 		if (callbacks->asioMessage) ProbeHostMessages(callbacks->asioMessage);
 	}
 
@@ -745,7 +731,6 @@ namespace flexasio {
 	void FlexASIO::DisposeBuffers()
 	{
 		if (!preparedState.has_value()) throw ASIOException(ASE_InvalidMode, "disposeBuffers() called before createBuffers()");
-		const std::lock_guard resetRequestLock(resetRequestMutex);
 		preparedState.reset();
 	}
 
@@ -827,6 +812,16 @@ namespace flexasio {
 		return result;
 	}
 
+	void FlexASIO::PreparedState::OnConfigChange() {
+		Log() << "Issuing reset request due to config change";
+		try {
+			RequestReset();
+		}
+		catch (const std::exception& exception) {
+			Log() << "Reset request failed: " << ::dechamps_cpputil::GetNestedExceptionMessage(exception);
+		}
+	}
+
 	PaStreamCallbackResult FlexASIO::PreparedState::RunningState::StreamCallback(const void *input, void *output, unsigned long frameCount, const PaStreamCallbackTimeInfo *timeInfo, PaStreamCallbackFlags statusFlags)
 	{
 		auto currentSamplePosition = samplePosition.load();
@@ -959,18 +954,6 @@ namespace flexasio {
 		outputReadyCondition.notify_all();
 	}
 
-	void FlexASIO::RequestReset() {
-		Log() << "Handling reset request";
-
-		const std::lock_guard resetRequestLock(resetRequestMutex);
-		if (!preparedState.has_value()) {
-			Log() << "No prepared state, will issue reset later";
-			resetRequested = true;
-			return;
-		}
-		preparedState->RequestReset();
-	}
-
 	void FlexASIO::PreparedState::RequestReset() {
 		if (!callbacks.asioMessage || Message(callbacks.asioMessage, kAsioSelectorSupported, kAsioResetRequest, nullptr, nullptr) != 1)
 			throw ASIOException(ASE_InvalidMode, "reset requests are not supported");

diff --git a/src/flexasio/FlexASIO/flexasio.h b/src/flexasio/FlexASIO/flexasio.h
@@ -175,6 +175,8 @@ namespace flexasio {
 
 			static int StreamCallback(const void *input, void *output, unsigned long frameCount, const PaStreamCallbackTimeInfo *timeInfo, PaStreamCallbackFlags statusFlags, void *userData) throw();
 
+			void OnConfigChange();
+
 			FlexASIO& flexASIO;
 			const ASIOSampleRate sampleRate;
 			const ASIOCallbacks callbacks;
@@ -193,6 +195,7 @@ namespace flexasio {
 			// During steady-state operation, runningState just points to *ownedRunningState.
 			RunningState* runningState = nullptr;
 			std::optional<RunningState> ownedRunningState;
+			ConfigLoader::Watcher configWatcher;
 		};
 
 		static const SampleType float32;
@@ -212,10 +215,8 @@ namespace flexasio {
 
 		OpenStreamResult OpenStream(bool inputEnabled, bool outputEnabled, double sampleRate, unsigned long framesPerBuffer, PaStreamCallback callback, void* callbackUserData);
 
-		void RequestReset();
-
 		const HWND windowHandle = nullptr;
-		const ConfigLoader configLoader{ [this] { RequestReset(); } };
+		const ConfigLoader configLoader;
 		const Config& config = configLoader.Initial();
 
 		PortAudioDebugRedirector portAudioDebugRedirector;
@@ -233,9 +234,6 @@ namespace flexasio {
 		bool sampleRateWasAccessed = false;
 		bool hostSupportsOutputReady = false;
 
-		std::mutex resetRequestMutex;
-		bool resetRequested = false;
-
 		std::optional<PreparedState> preparedState;
 	};