fixed SF# 3522906: Unregistering handlers from SocketReactor

CHANGELOG

@@ -1,5 +1,16 @@
 This is the changelog file for the POCO C++ Libraries.
 
+
+Release 1.5.0 (2012-07-04)
+==========================
+
+- added JSON
+- added Util::JSONConfiguration
+- added PDF
+- added PRoGen
+- added FIFOBuffer
+- fixed SF# 3522906: Unregistering handlers from SocketReactor
+ 
 Release 1.4.4 (2012-04-??)
 ==========================
 

Foundation/include/Poco/NotificationCenter.h

@@ -118,6 +118,9 @@ public:
 	void removeObserver(const AbstractObserver& observer);
 		/// Unregisters an observer with the NotificationCenter.
 
+	bool hasObserver(const AbstractObserver& observer) const;
+		/// Returns true if the observer is registered with this NotificationCenter.
+
 	void postNotification(Notification::Ptr pNotification);
 		/// Posts a notification to the NotificationCenter.
 		/// The NotificationCenter then delivers the notification

Foundation/src/NotificationCenter.cpp

@@ -76,6 +76,16 @@ void NotificationCenter::removeObserver(const AbstractObserver& observer)
 }
 
 
+bool NotificationCenter::hasObserver(const AbstractObserver& observer) const
+{
+	Mutex::ScopedLock lock(_mutex);
+	for (ObserverList::const_iterator it = _observers.begin(); it != _observers.end(); ++it)
+		if (observer.equals(**it)) return true;
+
+	return false;
+}
+
+
 void NotificationCenter::postNotification(Notification::Ptr pNotification)
 {
 	poco_check_ptr (pNotification);

Foundation/testsuite/src/NotificationCenterTest.cpp

@@ -71,13 +71,16 @@ void NotificationCenterTest::test1()
 void NotificationCenterTest::test2()
 {
 	NotificationCenter nc;
-	nc.addObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle1));
+	Observer<NotificationCenterTest, Notification> o(*this, &NotificationCenterTest::handle1);
+	nc.addObserver(o);
+	assert (nc.hasObserver(o));
 	assert (nc.hasObservers());
 	assert (nc.countObservers() == 1);
 	nc.postNotification(new Notification);
 	assert (_set.size() == 1);
 	assert (_set.find("handle1") != _set.end());
 	nc.removeObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle1));
+	assert (!nc.hasObserver(o));
 	assert (!nc.hasObservers());
 	assert (nc.countObservers() == 0);
 }
@@ -86,8 +89,12 @@ void NotificationCenterTest::test2()
 void NotificationCenterTest::test3()
 {
 	NotificationCenter nc;
-	nc.addObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle1));
-	nc.addObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle2));
+	Observer<NotificationCenterTest, Notification> o1(*this, &NotificationCenterTest::handle1);
+	Observer<NotificationCenterTest, Notification> o2(*this, &NotificationCenterTest::handle2);
+	nc.addObserver(o1);
+	assert (nc.hasObserver(o1));
+	nc.addObserver(o2);
+	assert (nc.hasObserver(o2));
 	assert (nc.hasObservers());
 	assert (nc.countObservers() == 2);
 	nc.postNotification(new Notification);
@@ -95,7 +102,9 @@ void NotificationCenterTest::test3()
 	assert (_set.find("handle1") != _set.end());
 	assert (_set.find("handle2") != _set.end());
 	nc.removeObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle1));
+	assert (!nc.hasObserver(o1));
 	nc.removeObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle2));
+	assert (!nc.hasObserver(o2));
 	assert (!nc.hasObservers());
 	assert (nc.countObservers() == 0);
 }
@@ -104,22 +113,31 @@ void NotificationCenterTest::test3()
 void NotificationCenterTest::test4()
 {
 	NotificationCenter nc;
-	nc.addObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle1));
-	nc.addObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle2));
+	Observer<NotificationCenterTest, Notification> o1(*this, &NotificationCenterTest::handle1);
+	Observer<NotificationCenterTest, Notification> o2(*this, &NotificationCenterTest::handle2);
+	nc.addObserver(o1);
+	assert (nc.hasObserver(o1));
+	nc.addObserver(o2);
+	assert (nc.hasObserver(o2));
 	nc.postNotification(new Notification);
 	assert (_set.size() == 2);
 	assert (_set.find("handle1") != _set.end());
 	assert (_set.find("handle2") != _set.end());
 	nc.removeObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle1));
+	assert (!nc.hasObserver(o1));
 	nc.removeObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle2));
+	assert (!nc.hasObserver(o2));
 	_set.clear();
 	nc.postNotification(new Notification);
 	assert (_set.empty());
-	nc.addObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle3));
+	Observer<NotificationCenterTest, Notification> o3(*this, &NotificationCenterTest::handle3);
+	nc.addObserver(o3);
+	assert (nc.hasObserver(o3));
 	nc.postNotification(new Notification);
 	assert (_set.size() == 1);
 	assert (_set.find("handle3") != _set.end());
 	nc.removeObserver(Observer<NotificationCenterTest, Notification>(*this, &NotificationCenterTest::handle3));
+	assert (!nc.hasObserver(o3));
 }
 
 

Net/include/Poco/Net/SocketNotifier.h

@@ -69,7 +69,10 @@ public:
 		/// Adds the given observer. 
 		
 	void removeObserver(SocketReactor* pReactor, const Poco::AbstractObserver& observer);
-		/// Removes the given observer.
+		/// Removes the given observer. 
+		
+	bool hasObserver(const Poco::AbstractObserver& observer) const;
+		/// Returns true if the given observer is registered.
 		
 	bool accepts(SocketNotification* pNotification);
 		/// Returns true if there is at least one observer for the given notification.
@@ -105,6 +108,12 @@ inline bool SocketNotifier::accepts(SocketNotification* pNotification)
 }
 
 
+inline bool SocketNotifier::hasObserver(const Poco::AbstractObserver& observer) const
+{
+	return _nc.hasObserver(observer);
+}
+
+
 inline bool SocketNotifier::hasObservers() const
 {
 	return _nc.hasObservers();

Net/include/Poco/Net/SocketReactor.h

@@ -169,6 +169,9 @@ public:
 		///     Poco::Observer<MyEventHandler, SocketNotification> obs(*this, &MyEventHandler::handleMyEvent);
 		///     reactor.addEventHandler(obs);
 
+	bool hasEventHandler(const Socket& socket, const Poco::AbstractObserver& observer);
+		/// Returns true if the observer is reistered with SocketReactor for the given socket.
+
 	void removeEventHandler(const Socket& socket, const Poco::AbstractObserver& observer);
 		/// Unregisters an event handler with the SocketReactor.
 		///

Net/src/SocketReactor.cpp

@@ -183,7 +183,26 @@ void SocketReactor::addEventHandler(const Socket& socket, const Poco::AbstractOb
 		}
 		else pNotifier = it->second;
 	}
-	pNotifier->addObserver(this, observer);
+	if (!pNotifier->hasObserver(observer))
+		pNotifier->addObserver(this, observer);
+}
+
+
+bool SocketReactor::hasEventHandler(const Socket& socket, const Poco::AbstractObserver& observer)
+{
+	NotifierPtr pNotifier;
+	{
+		FastMutex::ScopedLock lock(_mutex);
+	
+		EventHandlerMap::iterator it = _handlers.find(socket);
+		if (it != _handlers.end())
+		{
+			if (it->second->hasObserver(observer))
+				return true;
+		}
+	}
+
+	return false;
 }
 
 
@@ -192,21 +211,22 @@ void SocketReactor::removeEventHandler(const Socket& socket, const Poco::Abstrac
 	NotifierPtr pNotifier;
 	{
 		FastMutex::ScopedLock lock(_mutex);
-		
+	
 		EventHandlerMap::iterator it = _handlers.find(socket);
 		if (it != _handlers.end())
 		{
 			pNotifier = it->second;
-			if (pNotifier->countObservers() == 1)
+			if (pNotifier->hasObserver(observer) && pNotifier->countObservers() == 1)
 			{
 				_handlers.erase(it);
 			}
 		}
 	}
-	if (pNotifier)
+	if (pNotifier && pNotifier->hasObserver(observer))
 	{
 		pNotifier->removeObserver(this, observer);
 	}
+
 }
 
 

Net/testsuite/src/HTTPTestServer.cpp

@@ -108,7 +108,7 @@ void HTTPTestServer::run()
 					ss.shutdown();
 					Poco::Thread::sleep(1000);
 				}
-				catch (Poco::Exception& exc)
+				catch (Poco::Exception&)
 				{
 				}
 			}

Net/testsuite/src/SocketReactorTest.cpp

@@ -41,6 +41,7 @@
 #include "Poco/Net/ServerSocket.h"
 #include "Poco/Net/SocketAddress.h"
 #include "Poco/Observer.h"
+#include "Poco/Exception.h"
 #include <sstream>
 
 
@@ -56,6 +57,7 @@ using Poco::Net::WritableNotification;
 using Poco::Net::TimeoutNotification;
 using Poco::Net::ShutdownNotification;
 using Poco::Observer;
+using Poco::IllegalStateException;
 
 
 namespace
@@ -101,18 +103,31 @@ namespace
 	public:
 		ClientServiceHandler(StreamSocket& socket, SocketReactor& reactor):
 			_socket(socket),
-			_reactor(reactor)
+			_reactor(reactor),
+			_or(*this, &ClientServiceHandler::onReadable),
+			_ow(*this, &ClientServiceHandler::onWritable),
+			_ot(*this, &ClientServiceHandler::onTimeout)
 		{
 			_timeout = false;
-			_reactor.addEventHandler(_socket, Observer<ClientServiceHandler, ReadableNotification>(*this, &ClientServiceHandler::onReadable));
-			_reactor.addEventHandler(_socket, Observer<ClientServiceHandler, WritableNotification>(*this, &ClientServiceHandler::onWritable));
-			_reactor.addEventHandler(_socket, Observer<ClientServiceHandler, TimeoutNotification>(*this, &ClientServiceHandler::onTimeout));
+			_readableError = false;
+			_writableError = false;
+			_timeoutError = false;
+			checkReadableObserverCount(0);
+			_reactor.addEventHandler(_socket, _or);
+			checkReadableObserverCount(1);
+			checkWritableObserverCount(0);
+			_reactor.addEventHandler(_socket, _ow);
+			checkWritableObserverCount(1);
+			checkTimeoutObserverCount(0);
+			_reactor.addEventHandler(_socket, _ot);
+			checkTimeoutObserverCount(1);
+
 		}
 		
 		~ClientServiceHandler()
 		{
 		}
-		
+
 		void onReadable(ReadableNotification* pNf)
 		{
 			pNf->release();
@@ -124,7 +139,9 @@ namespace
 			}
 			else
 			{
+				checkReadableObserverCount(1);
 				_reactor.removeEventHandler(_socket, Observer<ClientServiceHandler, ReadableNotification>(*this, &ClientServiceHandler::onReadable));
+				checkReadableObserverCount(0);
 				_reactor.stop();
 				_data = _str.str();
 				delete this;
@@ -134,7 +151,9 @@ namespace
 		void onWritable(WritableNotification* pNf)
 		{
 			pNf->release();
+			checkWritableObserverCount(1);
 			_reactor.removeEventHandler(_socket, Observer<ClientServiceHandler, WritableNotification>(*this, &ClientServiceHandler::onWritable));
+			checkWritableObserverCount(0);
 			std::string data(1024, 'x');
 			_socket.sendBytes(data.data(), (int) data.length());
 			_socket.shutdownSend();
@@ -171,17 +190,68 @@ namespace
 			_closeOnTimeout = flag;
 		}
 		
+		static bool readableError()
+		{
+			return _readableError;
+		}
+		
+		static bool writableError()
+		{
+			return _writableError;
+		}
+		
+		static bool timeoutError()
+		{
+			return _timeoutError;
+		}
+
 	private:
-		StreamSocket       _socket;
-		SocketReactor&     _reactor;
-		std::stringstream  _str;
-		static std::string _data;
-		static bool        _timeout;
-		static bool        _closeOnTimeout;
+		void checkReadableObserverCount(std::size_t oro)
+		{
+			if (((oro == 0) && _reactor.hasEventHandler(_socket, _or)) ||
+				((oro > 0) && !_reactor.hasEventHandler(_socket, _or)))
+			{
+				_readableError = true;
+			}
+		}
+
+		void checkWritableObserverCount(std::size_t ow)
+		{
+			if (((ow == 0) && _reactor.hasEventHandler(_socket, _ow)) ||
+				((ow > 0) && !_reactor.hasEventHandler(_socket, _ow)))
+			{
+				_writableError = true;
+			}
+		}
+
+		void checkTimeoutObserverCount(std::size_t ot)
+		{
+			if (((ot == 0) && _reactor.hasEventHandler(_socket, _ot)) ||
+				((ot > 0) && !_reactor.hasEventHandler(_socket, _ot)))
+			{
+				_timeoutError = true;
+			}
+		}
+
+		StreamSocket                                         _socket;
+		SocketReactor&                                       _reactor;
+		Observer<ClientServiceHandler, ReadableNotification> _or;
+		Observer<ClientServiceHandler, WritableNotification> _ow;
+		Observer<ClientServiceHandler, TimeoutNotification>  _ot;
+		std::stringstream                                    _str;
+		static std::string                                   _data;
+		static bool                                          _readableError;
+		static bool                                          _writableError;
+		static bool                                          _timeoutError;
+		static bool                                          _timeout;
+		static bool                                          _closeOnTimeout;
 	};
 	
 	
 	std::string ClientServiceHandler::_data;
+	bool ClientServiceHandler::_readableError = false;
+	bool ClientServiceHandler::_writableError = false;
+	bool ClientServiceHandler::_timeoutError = false;
 	bool ClientServiceHandler::_timeout = false;
 	bool ClientServiceHandler::_closeOnTimeout = false;
 	
@@ -247,6 +317,9 @@ void SocketReactorTest::testSocketReactor()
 	reactor.run();
 	std::string data(ClientServiceHandler::data());
 	assert (data.size() == 1024);
+	assert (!ClientServiceHandler::readableError());
+	assert (!ClientServiceHandler::writableError());
+	assert (!ClientServiceHandler::timeoutError());
 }