"SocketReactor::addEventHandler" and "SocketReactor::removeEventHandler" must protect the access to "NotifierPtr pNotifier" #1709
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…er" must protect the access to "NotifierPtr pNotifier" 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 pocoproject#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 pocoproject#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 pocoproject#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 pocoproject#4 0x06cca72b in Poco::Net::SocketNotifier::addObserver (this=0xac75dc70, pReactor=0x8d27958, observer=@0xb5fb0cd0) at src/SocketNotifier.cpp:45 pocoproject#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.
|
Hi, this wasn't successfully merged into branch, because CI tests failed. So it isn't fixed in Release 1.8.0 (2017-11-10) as you mentioned in Changelog (https://raw.githubusercontent.com/pocoproject/poco/poco-1.9.0-release/CHANGELOG). But I saw in branch pocoproject:develop that it is completely reworked the way how to store and notify events... |
|
Socket notifier was not thread-safe to start with (see #2345), and there were other major changes to reactor (most notably, using PollSet to improve performance), fix will be in 1.9.1 For future contributions, keep in mind that develop (future 2.0) and 1.x are different lines of development, so any fix you want to see in 1.x releases should also be submitted there. we try to keep it synced, but sometimes things slip and not everything is easy or straightforward to move between branches |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is related to this issue: #1708
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.