Merge pull request 'simplify piprotectedvariable' (#197) from protected_var_refact into master

Reviewed-on: #197

libs/main/thread/piprotectedvariable.h

@@ -4,22 +4,22 @@
 //! \~english Thread-safe variable
 //! \~russian Потокобезопасная переменная
 /*
-	PIP - Platform Independent Primitives
-	Thread-safe variable
-	Ivan Pelipenko peri4ko@yandex.ru
+    PIP - Platform Independent Primitives
+    Thread-safe variable
+    Ivan Pelipenko peri4ko@yandex.ru
 
-	This program is free software: you can redistribute it and/or modify
-	it under the terms of the GNU Lesser General Public License as published by
-	the Free Software Foundation, either version 3 of the License, or
-	(at your option) any later version.
+    This program is free software: you can redistribute it and/or modify
+    it under the terms of the GNU Lesser General Public License as published by
+    the Free Software Foundation, either version 3 of the License, or
+    (at your option) any later version.
 
-	This program is distributed in the hope that it will be useful,
-	but WITHOUT ANY WARRANTY; without even the implied warranty of
-	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-	GNU Lesser General Public License for more details.
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU Lesser General Public License for more details.
 
-	You should have received a copy of the GNU Lesser General Public License
-	along with this program.  If not, see <http://www.gnu.org/licenses/>.
+    You should have received a copy of the GNU Lesser General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
 
 #ifndef PIPROTECTEDVARIABLE_H
@@ -34,21 +34,29 @@
 template<typename T>
 class PIP_EXPORT PIProtectedVariable {
 public:
+	//! \~english Constructs %PIProtectedVariable and initialize variable by value `v`.
+	//! \~russian Создает %PIProtectedVariable и инициализирует переменную значением `v`.
+	PIProtectedVariable(T v = T()): var(std::move(v)) {}
+
 	//! \~\brief
 	//! \~english Pointer-like wrapper returned by \a getRef() while the protected value remains locked.
 	//! \~russian Указателеподобная обертка, возвращаемая \a getRef(), пока защищенное значение остается заблокированным.
 	class PIP_EXPORT Pointer {
 		friend class PIProtectedVariable<T>;
+		NO_COPY_CLASS(Pointer);
+		Pointer & operator=(Pointer && other) = delete;
 
 	public:
-		//! \~english Copies wrapper state for access to the same protected value.
-		//! \~russian Копирует состояние обертки для доступа к тому же защищенному значению.
-		Pointer(const Pointer & v): pv(v.pv), counter(v.counter + 1) {}
+		//! \~english Move constructor - transfers ownership of the lock.
+		//! \~russian Конструктор перемещения - передает владение блокировкой.
+		Pointer(Pointer && other): pv(other.pv) { other.can_unlock = false; };
 
-		//! \~english Destroys wrapper and releases the mutex when it owns the original lock.
-		//! \~russian Уничтожает обертку и освобождает мьютекс, когда она владеет исходной блокировкой.
+		//! \~english Destroys wrapper and releases the mutex.
+		//! \~russian Уничтожает обертку и освобождает мьютекс.
 		~Pointer() {
-			if (counter == 0) pv.mutex.unlock();
+			if (can_unlock) {
+				pv.mutex.unlock();
+			}
 		}
 
 		//! \~english Returns pointer access to the protected value.
@@ -60,11 +68,11 @@ public:
 		T & operator*() { return pv.var; }
 
 	private:
-		Pointer() = delete;
-		Pointer(PIProtectedVariable<T> & v): pv(v) {}
+		explicit Pointer() = delete;
+		explicit Pointer(PIProtectedVariable<T> & v): pv(v) { pv.mutex.lock(); }
 
 		PIProtectedVariable<T> & pv;
-		int counter = 0;
+		bool can_unlock = true;
 	};
 
 	//! \~english Replaces the protected value with \a v.
@@ -76,10 +84,7 @@ public:
 
 	//! \~english Locks the value and returns wrapper-based access to it.
 	//! \~russian Блокирует значение и возвращает обертку для доступа к нему.
-	Pointer getRef() {
-		mutex.lock();
-		return Pointer(*this);
-	}
+	Pointer getRef() { return Pointer(*this); }
 
 	//! \~english Returns a copy of the protected value.
 	//! \~russian Возвращает копию защищенного значения.
@@ -98,7 +103,7 @@ public:
 
 private:
 	mutable PIMutex mutex;
-	T var;
+	T var = {};
 };
 
 

tests/CMakeLists.txt

@@ -37,3 +37,4 @@ pip_test(piobject)
 pip_test(client_server pip_client_server)
 pip_test(io)
 pip_test(system)
+pip_test(thread)

tests/thread/piprotectedvariable_test.cpp

@@ -0,0 +1,118 @@
+#include "piprotectedvariable.h"
+#include "pistring.h"
+#include "pithread.h"
+#include "piliterals_time.h"
+
+#include "gtest/gtest.h"
+#include <atomic>
+
+// Basic functionality tests
+TEST(PIProtectedVariable_Basic, BasicFunctionality) {
+	// Test basic set/get with different types
+	PIProtectedVariable<int> pvInt;
+	PIProtectedVariable<double> pvDouble;
+	PIProtectedVariable<PIString> pvString;
+
+	pvInt.set(123);
+	pvDouble.set(3.14159);
+	pvString.set(PIString("Hello, World!"));
+
+	EXPECT_EQ(pvInt.get(), 123);
+	EXPECT_DOUBLE_EQ(pvDouble.get(), 3.14159);
+	EXPECT_EQ(pvString.get(), PIString("Hello, World!"));
+
+	// Test operator=
+	pvInt = 999;
+	EXPECT_EQ(pvInt.get(), 999);
+
+	// Test getRef() with Pointer
+	struct TestStruct {
+		int x = 10;
+		int y = 20;
+		int getValue() const { return x + y; }
+	};
+
+	PIProtectedVariable<TestStruct> pvStruct;
+
+	auto ptr = pvStruct.getRef();
+	EXPECT_EQ(ptr->x, 10);
+	EXPECT_EQ(ptr->y, 20);
+	EXPECT_EQ(ptr->getValue(), 30);
+
+	// Modify through pointer
+	*ptr   = TestStruct();
+	ptr->x = 100;
+	EXPECT_EQ(pvStruct.get().x, 100);
+
+	// Test for Pointer
+	pvInt.set(42);
+	auto ptr1 = pvInt.getRef();
+	EXPECT_EQ(*ptr1, 42);
+	*ptr1 = 55;
+	EXPECT_EQ(*ptr1, 55);
+	auto ptr2 = std::move(ptr1);
+	EXPECT_EQ(*ptr2, 55);
+	*ptr2 = 100;
+	EXPECT_EQ(*ptr2, 100);
+	auto ptr3 = pvInt.getRef();
+	EXPECT_EQ(*ptr3, 100);
+	*ptr3 = 333;
+	EXPECT_EQ(*ptr3, 333);
+	EXPECT_EQ(pvInt.get(), 333);
+}
+
+// Thread safety tests
+TEST(PIProtectedVariable_ThreadSafety, ConcurrentReadWrite) {
+	PIProtectedVariable<int> pv;
+
+	std::atomic<int> writeCount(0);
+	std::atomic<int> readCount(0);
+	std::atomic<int> invalidReads(0);
+	const int NUM_ITERATIONS = 1000;
+	const int NUM_WRITERS    = 10;
+	const int NUM_READERS    = 20;
+	const int TOTAL_WRITES   = NUM_WRITERS * NUM_ITERATIONS;
+
+	// Collect thread handles for joining
+	PIVector<PIThread *> threads;
+
+	// Create writer threads
+	for (int i = 0; i < NUM_WRITERS; ++i) {
+		threads.push_back(new PIThread([&pv, &writeCount]() {
+			for (int j = 0; j < NUM_ITERATIONS; ++j) {
+				auto val = pv.getRef();
+				(*val)++;
+				auto val2 = pv.getRef();
+				writeCount++;
+				ASSERT_EQ(writeCount, *val2);
+			}
+		}));
+	}
+
+	// Create reader threads
+	for (int i = 0; i < NUM_READERS; ++i) {
+		threads.push_back(new PIThread([&pv, &invalidReads, &readCount]() {
+			for (int j = 0; j < NUM_ITERATIONS; ++j) {
+				auto val = pv.get();
+				readCount++;
+				// Value should always be in valid range [0, TOTAL_WRITES]
+				if (val < 0 || val > TOTAL_WRITES) {
+					invalidReads++;
+				}
+			}
+		}));
+	}
+
+	threads.forEach([](PIThread * & t) {t->startOnce();});
+	threads.forEach([](PIThread * & t) {t->waitForFinish(2_s);});
+	piDeleteAll(threads);
+
+	// Verify results
+	EXPECT_EQ(writeCount, TOTAL_WRITES);
+	EXPECT_EQ(readCount, NUM_READERS * NUM_ITERATIONS);
+	EXPECT_EQ(invalidReads, 0) << "All reads should return valid values in range [0, " << TOTAL_WRITES << "]";
+
+	// Final value should be TOTAL_WRITES
+	int finalVal = pv.get();
+	EXPECT_EQ(finalVal, TOTAL_WRITES);
+}