"SocketReactor::addEventHandler" and "SocketReactor::removeEventHandl…

…er" must protect the access to "NotifierPtr pNotifier" (#1709)

Those two methods already use FastMutex::ScopedLock lock(_mutex), but
the scope is not large enough to protect "NotifierPtr pNotifier" that is
accessed by calling SocketNotifier::addObserver() and
SocketNotifier::removeObserver().

It is mentioned in SocketReator.h that it is safe to call
addEventHandler() and removeEventHandler() from another thread while the
SocketReactor is running. My current use of the SocketReactor
encountered an issue where the SocketNotifier::_events has been
corrupted by a concurent write access done by
SocketReactor::addEventHandler() and
SocketReactor::removeEventHandler().
The call stack show that the SocketReactor::addEventHandler is stuck in
a while loop in gcc/libstdc++/tree.cc Rb_tree_insert_and_rebalance()

I clearly see in my logs that it happened while my
SocketConnector::unregisterConnector() and
SocketConnector::registerConnector() were called by two different
threads.

#0 0x00a80a7b in std::_Rb_tree_insert_and_rebalance () from
/usr/lib/libstdc++.so.6
#1 0x06ccb430 in std::_Rb_tree<Poco::Net::SocketNotification*,
Poco::Net::SocketNotification*,
std::_IdentityPoco::Net::SocketNotification*,
std::lessPoco::Net::SocketNotification*,
std::allocatorPoco::Net::SocketNotification* >::_M_insert
(this=0xac75dc90, __x=0x0, __p=0xac7ed0c8, __v=@0xb5fb0c40) at
/usr/lib/gcc/i386-redhat-linux/3.4.6/../../../../include/c++/3.4.6/bits/stl_tree.h:816
#2 0x06ccb15d in std::_Rb_tree<Poco::Net::SocketNotification*,
Poco::Net::SocketNotification*,
std::_IdentityPoco::Net::SocketNotification*,
std::lessPoco::Net::SocketNotification*,
std::allocatorPoco::Net::SocketNotification* >::insert_equal
(this=0xac75dc90, __v=@0xb5fb0c40) at
/usr/lib/gcc/i386-redhat-linux/3.4.6/../../../../include/c++/3.4.6/bits/stl_tree.h:858
#3 0x06ccad86 in std::multiset<Poco::Net::SocketNotification*,
std::lessPoco::Net::SocketNotification*,
std::allocatorPoco::Net::SocketNotification* >::insert (this=0xac75dc90,
__x=@0xb5fb0c40)
at
/usr/lib/gcc/i386-redhat-linux/3.4.6/../../../../include/c++/3.4.6/bits/stl_multiset.h:306
#4 0x06cca72b in Poco::Net::SocketNotifier::addObserver
(this=0xac75dc70, pReactor=0x8d27958, observer=@0xb5fb0cd0) at
src/SocketNotifier.cpp:45
#5 0x06cd060f in Poco::Net::SocketReactor::addEventHandler
(this=0x8d27958, socket=@0xac7f5a74, observer=@0xb5fb0cd0) at
src/SocketReactor.cpp:178

SocketReactor::run() is already protecting the access to the
SocketNotifier.

Net/src/SocketReactor.cpp

@@ -173,9 +173,10 @@ void SocketReactor::addEventHandler(const Socket& socket, const Poco::AbstractOb
 			_handlers[socket] = pNotifier;
 		}
 		else pNotifier = it->second;
-	}
-	if (!pNotifier->hasObserver(observer))
-		pNotifier->addObserver(this, observer);
+
+		if (!pNotifier->hasObserver(observer))
+			pNotifier->addObserver(this, observer);
+	}	
 }
 
 
@@ -212,12 +213,12 @@ void SocketReactor::removeEventHandler(const Socket& socket, const Poco::Abstrac
 				_handlers.erase(it);
 			}
 		}
-	}
-	if (pNotifier && pNotifier->hasObserver(observer))
-	{
-		pNotifier->removeObserver(this, observer);
-	}
 
+		if (pNotifier && pNotifier->hasObserver(observer))
+		{
+			pNotifier->removeObserver(this, observer);
+		}
+	}
 }