From 39e35c316dee7b199938e0190540ba98f8b26564 Mon Sep 17 00:00:00 2001
From: Andrew Auclair <andrewauclair@users.noreply.github.com>
Date: Mon, 20 Nov 2023 22:17:19 -0500
Subject: [PATCH] SplitterChannel::addChannel() should only add a channel once

SplitterChannel::addChannel() should only add a channel once to the internal vector. This prevents issues where the channel is accidentally added twice but only removed once because removeChannel stops at the first result. (#4270)
---
 Foundation/src/SplitterChannel.cpp       | 19 +++++++-----
 Foundation/testsuite/src/ChannelTest.cpp | 37 ++++++++++++++++++------
 Foundation/testsuite/src/ChannelTest.h   |  3 +-
 3 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/Foundation/src/SplitterChannel.cpp b/Foundation/src/SplitterChannel.cpp
index af1c26c16..2b8192a65 100644
--- a/Foundation/src/SplitterChannel.cpp
+++ b/Foundation/src/SplitterChannel.cpp
@@ -40,10 +40,15 @@ SplitterChannel::~SplitterChannel()
 
 void SplitterChannel::addChannel(Channel::Ptr pChannel)
 {
-	poco_check_ptr (pChannel);
+	poco_check_ptr(pChannel);
 
 	FastMutex::ScopedLock lock(_mutex);
-	_channels.push_back(pChannel);
+
+	// ensure that the channel is only added once
+	if (std::find(_channels.begin(), _channels.end(), pChannel) == _channels.end())
+	{
+		_channels.push_back(pChannel);
+	}
 }
 
 
@@ -51,13 +56,11 @@ void SplitterChannel::removeChannel(Channel::Ptr pChannel)
 {
 	FastMutex::ScopedLock lock(_mutex);
 
-	for (ChannelVec::iterator it = _channels.begin(); it != _channels.end(); ++it)
+	const auto it = std::find(_channels.begin(), _channels.end(), pChannel);
+
+	if (it != _channels.end())
 	{
-		if (*it == pChannel)
-		{
-			_channels.erase(it);
-			break;
-		}
+		_channels.erase(it);
 	}
 }
 
diff --git a/Foundation/testsuite/src/ChannelTest.cpp b/Foundation/testsuite/src/ChannelTest.cpp
index 3a7ec81e2..917ded521 100644
--- a/Foundation/testsuite/src/ChannelTest.cpp
+++ b/Foundation/testsuite/src/ChannelTest.cpp
@@ -35,7 +35,7 @@ using Poco::Thread;
 using Poco::Runnable;
 
 
-class SimpleFormatter: public Formatter
+class SimpleFormatter : public Formatter
 {
 public:
 	void format(const Message& msg, std::string& text)
@@ -50,7 +50,7 @@ public:
 class LogRunnable : public Runnable
 {
 public:
-	LogRunnable(AutoPtr<AsyncChannel> pAsync):
+	LogRunnable(AutoPtr<AsyncChannel> pAsync) :
 		_pAsync(pAsync),
 		_stop(false)
 	{
@@ -73,7 +73,7 @@ private:
 };
 
 
-ChannelTest::ChannelTest(const std::string& name): CppUnit::TestCase(name)
+ChannelTest::ChannelTest(const std::string& name) : CppUnit::TestCase(name)
 {
 }
 
@@ -84,16 +84,34 @@ ChannelTest::~ChannelTest()
 
 
 void ChannelTest::testSplitter()
+{
+	AutoPtr<TestChannel> pChannel1 = new TestChannel;
+	AutoPtr<TestChannel> pChannel2 = new TestChannel;
+	AutoPtr<SplitterChannel> pSplitter = new SplitterChannel;
+	pSplitter->addChannel(pChannel1);
+	pSplitter->addChannel(pChannel2);
+	Message msg;
+	pSplitter->log(msg);
+	assertTrue(pChannel1->list().size() == 1);
+	assertTrue(pChannel2->list().size() == 1);
+}
+
+void ChannelTest::testSplitterAddSameChannelTwice()
 {
 	AutoPtr<TestChannel> pChannel = new TestChannel;
 	AutoPtr<SplitterChannel> pSplitter = new SplitterChannel;
 	pSplitter->addChannel(pChannel);
 	pSplitter->addChannel(pChannel);
+
+	assertTrue(pSplitter->count() == 1);
+
 	Message msg;
 	pSplitter->log(msg);
-	assertTrue (pChannel->list().size() == 2);
-}
 
+	pSplitter->removeChannel(pChannel);
+
+	assertTrue(pSplitter->count() == 0);
+}
 
 void ChannelTest::testAsync()
 {
@@ -109,7 +127,7 @@ void ChannelTest::testAsync()
 	pAsync->close();
 	lr.stop();
 	t.join();
-	assertTrue (pChannel->list().size() >= 2);
+	assertTrue(pChannel->list().size() >= 2);
 }
 
 
@@ -120,8 +138,8 @@ void ChannelTest::testFormatting()
 	AutoPtr<FormattingChannel> pFormatterChannel = new FormattingChannel(pFormatter, pChannel);
 	Message msg("Source", "Text", Message::PRIO_INFORMATION);
 	pFormatterChannel->log(msg);
-	assertTrue (pChannel->list().size() == 1);
-	assertTrue (pChannel->list().begin()->getText() == "Source: Text");
+	assertTrue(pChannel->list().size() == 1);
+	assertTrue(pChannel->list().begin()->getText() == "Source: Text");
 }
 
 
@@ -143,7 +161,7 @@ void ChannelTest::testStream()
 	AutoPtr<FormattingChannel> pFormatterChannel = new FormattingChannel(pFormatter, pChannel);
 	Message msg("Source", "Text", Message::PRIO_INFORMATION);
 	pFormatterChannel->log(msg);
-	assertTrue (str.str().find("Source: Text") == 0);
+	assertTrue(str.str().find("Source: Text") == 0);
 }
 
 
@@ -162,6 +180,7 @@ CppUnit::Test* ChannelTest::suite()
 	CppUnit::TestSuite* pSuite = new CppUnit::TestSuite("ChannelTest");
 
 	CppUnit_addTest(pSuite, ChannelTest, testSplitter);
+	CppUnit_addTest(pSuite, ChannelTest, testSplitterAddSameChannelTwice);
 	CppUnit_addTest(pSuite, ChannelTest, testAsync);
 	CppUnit_addTest(pSuite, ChannelTest, testFormatting);
 	CppUnit_addTest(pSuite, ChannelTest, testConsole);
diff --git a/Foundation/testsuite/src/ChannelTest.h b/Foundation/testsuite/src/ChannelTest.h
index a8ecba0c4..08cc23f7f 100644
--- a/Foundation/testsuite/src/ChannelTest.h
+++ b/Foundation/testsuite/src/ChannelTest.h
@@ -18,13 +18,14 @@
 #include "CppUnit/TestCase.h"
 
 
-class ChannelTest: public CppUnit::TestCase
+class ChannelTest : public CppUnit::TestCase
 {
 public:
 	ChannelTest(const std::string& name);
 	~ChannelTest();
 
 	void testSplitter();
+	void testSplitterAddSameChannelTwice();
 	void testAsync();
 	void testFormatting();
 	void testConsole();