Fixed FlowStatus race condition in ChannelDataElement and fixed BufferLockFree implementation for the circular buffer case #117

meyerj · 2015-10-27T18:41:44Z

This pull request is part of a bigger effort to add support for new connection semantics to RTT (see #114).

Moved storage of the FlowStatus to the DataObjectInterface or BufferInterface implementations instead as unprotected member variables of ChannelDataElement
Decide whether to use an AtomicMWMRQueue (renamed from AtomicQueue) or an AtomicMWSRQueue in BufferLockFree depending on whether the buffer is circular or must support multiple reader threads.
For a circular buffer also the writer thread may consume elements if the buffer is full, so the queue has to support multiple readers. This was broken in previous versions of RTT v2 and could lead to buffer corruption on concurrent writer/reader access.
Added a boolean return value to DataObjectInterface::Set() in order to notify the caller whether setting the data object succeeded or not. It can only fail for DataObjectLockFree if too many readers try to access it concurrently.
Allow to construct uninitialized data objects or buffers and added a boolean argument to data_sample(...) to enforce reinitialization or not
Added base class base::DataObjectBase for all data object types, similar to existing base::BufferBase
Added new structs base::BufferBase::Options and base::DataObjectBase::Options to pass options to the constructors of buffer and data objects to avoid ambiguity
Store the ConnPolicy used to construct the data object or buffer in ChannelDataElement or ChannelBufferElement, respectively
Added multi-threaded tests for class TsPool, data objects and buffers

…rLockFree implementation for the circular buffer case * Moved storage of the FlowStatus to the DataObjectInterface or BufferInterface implementations instead as unprotected member variables of ChannelDataElement * Decide whether to use an AtomicMWMRQueue (renamed from AtomicQueue) or an AtomicMWSRQueue in BufferLockFree depending on whether the buffer is circular or must support multiple reader threads. For a circular buffer also the writer thread may consume elements if the buffer is full, so the queue has to support multiple readers. * Added a boolean return value to DataObjectInterface::Set() in order to notify the caller whether setting the DataObject succeeded or not. It can only fail for DataObjectLockFree. * Allow to construct uninitialized data objects or buffers and added a boolean argument to data_sample(...) to enforce reinitialization or not * Added base class DataObjectBase for all data object types * Added new structs base::BufferBase::Options and base::DataObjectBase::Options to pass options to the constructors of buffer and data objects to avoid ambiguity * Added a new struct internal::DataObjectLockFreeOptions that is passed to the constructor of DataObjectLockFree<T> objects to avoid ambiguity * Store the ConnPolicy used to construct the data object or buffer in ChannelDataElement or ChannelBufferElement, respectively * Added multi-threaded tests for TsPool, data objects and buffers Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

… errors in gcc <4.7 The using declaration cannot be used to introduce type names from templated base classes. This bug has been fixed in gcc 4.7. See also http://stackoverflow.com/questions/1071119/accessing-types-from-dependent-base-classes. Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

Conflicts: rtt/base/BufferLockFree.hpp rtt/base/BufferLocked.hpp rtt/base/BufferUnSync.hpp rtt/internal/ChannelBufferElement.hpp rtt/internal/ChannelDataElement.hpp

doudou · 2016-12-16T11:13:48Z

rtt/base/DataObjectLockFree.hpp

@@ -198,34 +237,77 @@ namespace RTT
             * not increment the counter on write_ptr+1). Hence, no
             * locking is needed.
             */
+
+            if (!initialized) {
+                types::TypeInfo *ti = types::Types()->getTypeInfo<DataType>();


.... I would suggest to at least add a way for the TypeInfo object to tell that the type is RT-safe by itself.

And a lot of Rock users really don't care that much about RT-safety. Having logs and output filled with errors that you really don't care about is really not that great. Could we at least demote this message to Info ? Or have a way to disable it ?

You are right that not having set a data sample is not an error for many types and the error message should be degraded to warning or info level. On the other side I never saw this triggering for the normal usage of the lock-free data object in port connections, at least not for local connections. The OutputPort always sets a sample on the connection in OutputPort.hpp:122. I would have to verify if this is also the case for remote connections and/or streams.

Did you actually see this message while testing this branch with Rock?

Did you actually see this message while testing this branch with Rock?

No. Was purely on a "reading review".

doudou · 2016-12-16T11:20:57Z

rtt/base/DataObjectLockFree.hpp

+        virtual void clear() {
+            if (!initialized) return;
+
+            PtrType reading;


Wouldn't it make sense to factor out this (complex) piece of logic into a acquireReadPointer or something like that ?

doudou · 2016-12-16T11:30:15Z

rtt/internal/ChannelBufferElement.hpp

            buffer->clear();
            base::ChannelElement<T>::clear();
        }

        virtual bool data_sample(param_t sample)
        {
-            buffer->data_sample(sample);
+            if (!buffer->data_sample(sample)) return false;


Is it on purpose that the call to the base class is bypassed if buffer->data_sample(sample) returns false ? I kind of fail to really see what possible consequences this could have...

The base class will forward the call to all subsequent channel elements. A failure to initialize the buffer must result in an error and usually this triggers a failure of the connection attempt and disconnect anyway. It would still be possible to forward the call down the pipeline to not depend on a certain behavior of the ConnectionFactory in case it would not disconnect.

All current implementations of BufferBase::data_sample(sample) always return true, so actually it was not even necessary to introduce a return value.

doudou · 2016-12-16T11:30:36Z

rtt/internal/ChannelBufferElement.hpp

+
+        /** Returns a pointer to the ConnPolicy that has been used to construct the underlying buffer.
+        */
+        virtual const ConnPolicy* getConnPolicy() const


Why a pointer (vs plain object or reference) ?

With #114 this method implements ChannelElementBase::getConnPolicy() and the default implementation in ChannelInterface.cpp:208 returns a null pointer. Not every channel element stores its connection policy. Only special implementations like data or buffer elements do.

The predecessor of this method was ChannelElementBase::getConnID(), which also returned a pointer that could be null.

doudou · 2016-12-16T11:32:01Z

rtt/base/BufferBase.cpp

+BufferBase::Options::Options(const ConnPolicy &policy)
+    : circular_(policy.type == ConnPolicy::CIRCULAR_BUFFER)
+    , max_threads_(2)
+//    , multiple_writers_(policy.read_policy == ReadShared)


Do you really mean to commit these commented-out code ?

Yes: This PR must be considered as a part of the bigger data flow refactoring in #114. The complete patch sets the multiple_writers_ and multiple_readers_ flags from the read_policy/write_policy fields of the connection policy. I only commented the initializers here because the new fields are not yet available without #118.

…s been written (fix orocos-toolchain#177) Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

… in DataObjectInterface<T>::Get(...) Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

…raits This commit reverts the addition of an extra overload DataObjectInterface::Get(T&, bool, bool copy_sample) in 0853ec0 in favor of a new method DataObjectInterface::data_sample() for reasons of consistency with BufferInterface::data_sample(). Furthermore it applies the usage of Boost call_traits consistently for both base classes and their children and deletes the DataType typedef. Another minor change involves the consultation of internal::DataSourceTypeInfo<T>::getType() to output the type name in an error log message in DataObjectLockFree::Set(param_t), which can handle unknown and pointer types internally and seems to be more in line with other usages within RTT. This partially reverts 0853ec0. Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

…Pool class ...and moved testMemoryPoolMultiThreaded implementation from member function to the test case. Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

…BuffersDataFlowTestSuite test suite Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

meyerj mentioned this pull request Oct 27, 2015

New dataflow semantics with shared input and output buffers #114

Merged

5 tasks

meyerj added this to the 2.9 milestone Oct 27, 2015

meyerj added 2 commits August 5, 2016 20:50

Merge branch 'master' into updated-dataflow-semantics-buffers

8d7c478

Conflicts: rtt/base/BufferLockFree.hpp rtt/base/BufferLocked.hpp rtt/base/BufferUnSync.hpp rtt/internal/ChannelBufferElement.hpp rtt/internal/ChannelDataElement.hpp

doudou reviewed Dec 16, 2016

View reviewed changes

meyerj added 3 commits December 21, 2016 18:24

Do not copy the sample when reading a data object in case it never ha…

62d5462

…s been written (fix orocos-toolchain#177) Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

Fixed InputPort<T>::getDataSample() by introducing a copy_sample flag…

0853ec0

… in DataObjectInterface<T>::Get(...) Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

tests: fixed missing namespace specifier for shared_ptr in buffers_test

c54cc64

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

meyerj mentioned this pull request Oct 21, 2017

getDataSample() always returns container with zero size #231

Closed

meyerj added 3 commits October 26, 2017 21:10

tests: refactored atomic queue tests in buffers_test using the Thread…

71016c7

…Pool class ...and moved testMemoryPoolMultiThreaded implementation from member function to the test case. Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

tests: fixed max_threads parameterization of buffer instances in the …

daf6253

…BuffersDataFlowTestSuite test suite Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

meyerj merged commit ef5ffc3 into orocos-toolchain:master May 8, 2019

meyerj deleted the updated-dataflow-semantics-buffers branch May 8, 2019 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed FlowStatus race condition in ChannelDataElement and fixed BufferLockFree implementation for the circular buffer case #117

Fixed FlowStatus race condition in ChannelDataElement and fixed BufferLockFree implementation for the circular buffer case #117

meyerj commented Oct 27, 2015

doudou Dec 16, 2016

meyerj Dec 16, 2016

doudou Dec 16, 2016

doudou Dec 16, 2016

doudou Dec 16, 2016

meyerj Dec 16, 2016

doudou Dec 16, 2016

meyerj Dec 16, 2016

doudou Dec 16, 2016

meyerj Dec 16, 2016

Fixed FlowStatus race condition in ChannelDataElement and fixed BufferLockFree implementation for the circular buffer case #117

Fixed FlowStatus race condition in ChannelDataElement and fixed BufferLockFree implementation for the circular buffer case #117

Conversation

meyerj commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment