Skip to content

Prevent possible data race in access to Timer::_periodicInerval#646

Merged
aleks-f merged 1 commit intopocoproject:developfrom
adriaan42:develop
Jan 26, 2015
Merged

Prevent possible data race in access to Timer::_periodicInerval#646
aleks-f merged 1 commit intopocoproject:developfrom
adriaan42:develop

Conversation

@adriaan42
Copy link
Copy Markdown
Contributor

Hi,

I have another Helgrind report. This time it involves access to Timer::_periodicInterval.
In Timer::run() there is one unprotected read of this variable.

As alternative to the proposed patch, this would also work:

@@ -186,9 +186,7 @@ void Timer::run()

                if (_wakeUp.tryWait(sleep))
                {
-                       Poco::FastMutex::ScopedLock lock(_mutex);
                        _nextInvocation.update();
-                       interval = _periodicInterval;
                }
                else
                {
@@ -208,8 +206,9 @@ void Timer::run()
                        {
                                Poco::ErrorHandler::handle();
                        }
-                       interval = _periodicInterval;
                }
+               Poco::FastMutex::ScopedLock lock(_mutex);
+               interval = _periodicInterval;
                _nextInvocation += static_cast<Clock::ClockVal>(interval)*1000;
                _skipped = 0;
        }

Or this, which only protects the read from _periodicInterval. If my understanding is right, _nextInvocation does not need synchronization, as it is only accessed by the timer thread itself:

@@ -186,9 +186,7 @@ void Timer::run()

                if (_wakeUp.tryWait(sleep))
                {
-                       Poco::FastMutex::ScopedLock lock(_mutex);
                        _nextInvocation.update();
-                       interval = _periodicInterval;
                }
                else
                {
@@ -208,8 +206,10 @@ void Timer::run()
                        {
                                Poco::ErrorHandler::handle();
                        }
-                       interval = _periodicInterval;
                }
+               _mutex.lock();
+               interval = _periodicInterval;
+               _mutex.unlock();
                _nextInvocation += static_cast<Clock::ClockVal>(interval)*1000;
                _skipped = 0;
        }

Thanks,
Adriaan

==4273== Possible data race during write of size 8 at 0x808BF70 by thread #1
==4273== Locks held: 2, at addresses 0x808C150 0x808C050
==4273==    at 0x503217D: Poco::Timer::stop() (Timer.cpp:100)
==4273==    by 0x58E3985: Poco::Data::SessionPool::shutdown() (SessionPool.cpp:285)
==4273==    by 0x58E1FE0: Poco::Data::SessionPool::~SessionPool() (SessionPool.cpp:46)
==4273==    by 0x58E21F7: Poco::Data::SessionPool::~SessionPool() (SessionPool.cpp:52)
==4273==    by 0x5278F4: void boost::checked_delete<Poco::Data::SessionPool>(Poco::Data::SessionPool*) (checked_delete.hpp:34)
==4273==    by 0x527BCB: boost::detail::sp_counted_impl_p<Poco::Data::SessionPool>::dispose() (sp_counted_impl.hpp:78)
==4273==    by 0x50AE61: boost::detail::sp_counted_base::release() (sp_counted_base_gcc_x86.hpp:146)
==4273==    by 0x50AF0E: boost::detail::shared_count::~shared_count() (shared_count.hpp:371)
==4273==    by 0x525205: boost::shared_ptr<Poco::Data::SessionPool>::~shared_ptr() (shared_ptr.hpp:328)
[...]
==4273== 
==4273== This conflicts with a previous read of size 8 by thread #2
==4273== Locks held: none
==4273==    at 0x503261E: Poco::Timer::run() (Timer.cpp:211)
==4273==    by 0x502E4B2: Poco::PooledThread::run() (ThreadPool.cpp:200)
==4273==    by 0x502B8A0: Poco::(anonymous namespace)::RunnableHolder::run() (Thread.cpp:57)
==4273==    by 0x502B576: Poco::ThreadImpl::runnableEntry(void*) (Thread_POSIX.cpp:344)
==4273==    by 0x4C2F056: mythread_wrapper (hg_intercepts.c:234)
==4273==    by 0x6E910A3: start_thread (pthread_create.c:309)
==4273==    by 0x6BC5CCC: clone (clone.S:111)

@obiltschnig obiltschnig added this to the Release 1.6.1 milestone Dec 20, 2014
@obiltschnig obiltschnig self-assigned this Dec 20, 2014
@obiltschnig
Copy link
Copy Markdown
Member

postponing this for 1.6.1

aleks-f added a commit that referenced this pull request Jan 26, 2015
Prevent possible data race in access to Timer::_periodicInerval
@aleks-f aleks-f merged commit c14f8a8 into pocoproject:develop Jan 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants