pocoproject/poco — pull request #646 by adriaan42 (2014-12-19 10:20:26+00:00)
Title: Prevent possible data race in access to Timer::_periodicInerval

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(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(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*) (checked_delete.hpp:34)
==4273== by 0x527BCB: boost::detail::sp_counted_impl_p::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::~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)
```