Fix race conditions when accessing variables

lib/variables/variable-string.cpp

@@ -9,11 +9,12 @@ void StringVariable::Resolve() const
 		_resolvedValue = _value;
 		return;
 	}
-	if (_lastResolve == GetLastVariableChangeTime()) {
+	const auto lastChange = GetLastVariableChangeTime();
+	if (_lastResolve == lastChange) {
 		return;
 	}
 	_resolvedValue = SubstitueVariables(_value);
-	_lastResolve = GetLastVariableChangeTime();
+	_lastResolve = lastChange;
 }
 
 StringVariable::operator std::string() const
@@ -81,7 +82,7 @@ std::string SubstitueVariables(std::string str)
 		const auto &variable = std::dynamic_pointer_cast<Variable>(v);
 		const std::string pattern = "${" + variable->Name() + "}";
 		if (ReplaceAll(str, pattern, variable->Value(false))) {
-			variable->UpdateLastUsed();
+			variable->MarkAsUsed();
 		}
 	}
 	return str;

lib/variables/variable.cpp

@@ -12,16 +12,23 @@ static std::deque<std::shared_ptr<Item>> variables;
 
 // Keep track of the last time a variable was changed to save some work when
 // when resolving strings containing variables, etc.
+static std::mutex lastVariableChangeMutex;
 static std::chrono::high_resolution_clock::time_point lastVariableChange{};
 
+static void setLastVariableChangeTime()
+{
+	std::lock_guard<std::mutex> lock(lastVariableChangeMutex);
+	lastVariableChange = std::chrono::high_resolution_clock::now();
+}
+
 Variable::Variable() : Item()
 {
-	lastVariableChange = std::chrono::high_resolution_clock::now();
+	setLastVariableChangeTime();
 }
 
 Variable::~Variable()
 {
-	lastVariableChange = std::chrono::high_resolution_clock::now();
+	setLastVariableChangeTime();
 }
 
 void Variable::Load(obs_data_t *obj)
@@ -37,7 +44,7 @@ void Variable::Load(obs_data_t *obj)
 		SetValue(_defaultValue);
 	}
 
-	lastVariableChange = std::chrono::high_resolution_clock::now();
+	setLastVariableChangeTime();
 }
 
 void Variable::Save(obs_data_t *obj) const
@@ -45,8 +52,11 @@ void Variable::Save(obs_data_t *obj) const
 	Item::Save(obj);
 	obs_data_set_int(obj, "saveAction", static_cast<int>(_saveAction));
 
-	if (_saveAction == SaveAction::SAVE) {
-		obs_data_set_string(obj, "value", _value.c_str());
+	{
+		std::lock_guard<std::mutex> lock(_mutex);
+		if (_saveAction == SaveAction::SAVE) {
+			obs_data_set_string(obj, "value", _value.c_str());
+		}
 	}
 
 	obs_data_set_string(obj, "defaultValue", _defaultValue.c_str());
@@ -62,6 +72,18 @@ std::string Variable::Value(bool updateLastUsed) const
 	return _value;
 }
 
+std::string Variable::GetPreviousValue() const
+{
+	std::lock_guard<std::mutex> lock(_mutex);
+	return _previousValue;
+}
+
+int Variable::GetValueChangeCount() const
+{
+	std::lock_guard<std::mutex> lock(_mutex);
+	return _valueChangeCount;
+}
+
 std::optional<double> Variable::DoubleValue() const
 {
 	return GetDouble(Value());
@@ -79,8 +101,11 @@ void Variable::SetValue(const std::string &value)
 	_value = value;
 
 	UpdateLastUsed();
-	UpdateLastChanged();
-	lastVariableChange = std::chrono::high_resolution_clock::now();
+	if (_previousValue != _value) {
+		_lastChanged = std::chrono::high_resolution_clock::now();
+		++_valueChangeCount;
+	}
+	setLastVariableChangeTime();
 }
 
 void Variable::SetValue(double value)
@@ -90,6 +115,7 @@ void Variable::SetValue(double value)
 
 std::optional<uint64_t> Variable::GetSecondsSinceLastUse() const
 {
+	std::lock_guard<std::mutex> lock(_mutex);
 	if (_lastUsed.time_since_epoch().count() == 0) {
 		return {};
 	}
@@ -101,6 +127,7 @@ std::optional<uint64_t> Variable::GetSecondsSinceLastUse() const
 
 std::optional<uint64_t> Variable::GetSecondsSinceLastChange() const
 {
+	std::lock_guard<std::mutex> lock(_mutex);
 	if (_lastChanged.time_since_epoch().count() == 0) {
 		return {};
 	}
@@ -116,12 +143,10 @@ void Variable::UpdateLastUsed() const
 	_lastUsed = std::chrono::high_resolution_clock::now();
 }
 
-void Variable::UpdateLastChanged()
+void Variable::MarkAsUsed() const
 {
-	if (_previousValue != _value) {
-		_lastChanged = std::chrono::high_resolution_clock::now();
-		++_valueChangeCount;
-	}
+	std::lock_guard<std::mutex> lock(_mutex);
+	UpdateLastUsed();
 }
 
 static void populateSaveActionSelection(QComboBox *list)
@@ -206,7 +231,7 @@ bool VariableSettingsDialog::AskForSettings(QWidget *parent, Variable &settings)
 		dialog._defaultValue->toPlainText().toStdString();
 	settings._saveAction =
 		static_cast<Variable::SaveAction>(dialog._save->currentIndex());
-	lastVariableChange = std::chrono::high_resolution_clock::now();
+	setLastVariableChangeTime();
 
 	return true;
 }
@@ -461,6 +486,7 @@ void ImportVariables(obs_data_t *data)
 
 std::chrono::high_resolution_clock::time_point GetLastVariableChangeTime()
 {
+	std::lock_guard<std::mutex> lock(lastVariableChangeMutex);
 	return lastVariableChange;
 }
 

lib/variables/variable.hpp

@@ -34,16 +34,15 @@ public:
 	EXPORT std::string Value(bool updateLastUsed = true) const;
 	EXPORT std::optional<double> DoubleValue() const;
 	EXPORT std::optional<int> IntValue() const;
-	std::string GetPreviousValue() const { return _previousValue; };
+	std::string GetPreviousValue() const;
 	std::string GetDefaultValue() const { return _defaultValue; }
 	EXPORT void SetValue(const std::string &value);
 	void SetValue(double value);
 	SaveAction GetSaveAction() const { return _saveAction; }
-	int GetValueChangeCount() const { return _valueChangeCount; }
+	int GetValueChangeCount() const;
 	std::optional<uint64_t> GetSecondsSinceLastUse() const;
 	std::optional<uint64_t> GetSecondsSinceLastChange() const;
-	void UpdateLastUsed() const;
-	void UpdateLastChanged();
+	void MarkAsUsed() const;
 
 private:
 	SaveAction _saveAction = SaveAction::DONT_SAVE;
@@ -55,6 +54,8 @@ private:
 	mutable std::chrono::high_resolution_clock::time_point _lastChanged;
 	mutable std::mutex _mutex;
 
+	void UpdateLastUsed() const;
+
 	friend VariableSelection;
 	friend VariableSettingsDialog;
 };

tests/test-variable.cpp

@@ -40,7 +40,7 @@ TEST_CASE("Variable", "[variable]")
 	variable.Value(false);
 	REQUIRE(*variable.GetSecondsSinceLastUse() > 1);
 
-	variable.UpdateLastUsed();
+	variable.MarkAsUsed();
 	REQUIRE(*variable.GetSecondsSinceLastUse() == 0);
 
 	variable.SetValue(123);