If at startup the requested interface cannot be found (or no suitable
interface exists), the code failed to release all memory allocated in
the process.
Signed-off-by: Erik Boasson <eb@ilities.com>
This commit changes a few things in the config handling:
* When reading the configuration from multiple sources, a source can now
override settings already set by a preceding source for settings that
are not lists. Previously, trying to change the value of a setting in a
subsequence file would be considered an error, just like trying to set
the value of a particular setting multiple times in a single
configuration file.
* A configuration fragment in CYCLONEDDS_URI now no longer requires the
CycloneDDS top-level tag to be specified. If it is missing it will be
assumed. This is only true for configuration fragments contained in
CYCLONEDDS_URI, not for data read from a file.
* A configuration fragment in CYCLONEDDS_URI no longer requires that all
elements are properly closed: a missing close tag is treated as-if it
is the end of the fragment and any elements are implicitly closed.
Again this does not apply to files.
* The configuration dump now lists explicitly which sources affected
each setting, with a default value indicated by an empty set.
The result of the latter two is that one can almost pretend that it is a
sane format instead of XML. For example, if one would like to override
tracing settings, one could just write:
CYCLONEDDS_URI="$CYCLONEDDS_URI,<Tracing><Verbosity>finest"
Signed-off-by: Erik Boasson <eb@ilities.com>
The primary reason is that this allows the implementator of the sertopic
to freely select an allocation strategy, instead of being forced to
allocate the sertopic itself and the names it contains in the common
header with ddsrt_malloc. The secondary reason is that it brings it in
line with the serdata.
Signed-off-by: Erik Boasson <eb@ilities.com>
The thread state management used by the GC and the liveliness monitoring
lazily creates entries for application threads that call (certain)
Cyclone API functions. It arranges for the entry allocated to such a
thread to be cleared once the thread terminates.
This means that if such a thread still exists once the last participant
is deleted (and Cyclone deinitializes), the corresponding thread entry
still exists as well. The assertion that all (known) threads must have
stopped on shutting down Cyclone is therefore incorrect.
This commit introduces a special state for thread entries that were
created lazily. It does monitor that they do not stay in the "awake"
state for too long, but otherwise doesn't care much about them.
Signed-off-by: Erik Boasson <eb@ilities.com>
The Cyclone DDS configuration is in principle an XML document, but it is
possible to write configuration fragments directly in the CYCLONEDDS_URI
environment variable. In that case, it is quite annoying to have to
enter the full closing tags all the time, and so it now allows closing
elements with a simple </> when not reading them from a file.
While it would be trivial to also allow this when reading the
configuration from a file, it seems that promulgating invalid XML would
be bad form ... and besides, in that case editors can help keep
everything in order.
Signed-off-by: Erik Boasson <eb@ilities.com>
The entirely historical "DDSI2E" element within the CycloneDDS
configuration element is herewith eliminated. All settings contained in
that element (such as General, Discovery, Tracing) are now subelements
of the CycloneDDS top-level element. Old configurations continue to
work but will print a deprecation warning:
//CycloneDDS/DDSI2E: settings moved to //CycloneDDS
Any warnings/errors related for an element //CycloneDDS/DDSI2E/x will be
reported as errors for the new location, that is, for //CycloneDDS/x.
As the "settings moved" warning always precedes any other such warning,
confusion will hopefully be avoided.
Signed-off-by: Erik Boasson <eb@ilities.com>
The Internal/SuppressSPDPMulticast setting was one of several ways to
prevent the sending of participant discovery multicast messages while
still allowing multicast to be used for data communications. That
functionality has long since been subsumed by the AllowMulticast setting
(AllowMulticast = spdp,amc & Internal/SuppressSPDPMulticast is
equivalent to AllowMulticast = amc).
Signed-off-by: Erik Boasson <eb@ilities.com>
These settings all constitute settings from the long history of the DDSI
stack predating Eclipse Cyclone DDS and can reasonably be presumed never
to have been used in Cyclone. Their removal is therefore not expected
to break backwards compatibility (which would be anyway be limited to
Cyclone complaining about undefined settings at startup):
* Tracing/Timestamps[@absolute]: has always been ignored
* Tracing/Timestamps: has always been ignored
* General/EnableLoopback: ignored for quite some time, before that
changing it from the default resulted in crashes.
* General/StartupModeDuration: it did what it advertised (retain data in
the history caches of volatile writers as-if they were transient-local
with a durability history setting of keep-last 1 for the first few
seconds after startup of the DDSI stack) but had no purpose other than
complicating things as the volatile readers ignored the data anyway.
* General/StartupModeCoversTransient: see previous -- and besides,
transient data is not supported yet in Cyclone.
* Compatibility/RespondToRtiInitZeroAckWithInvalidHeartbeat: arguably a
good setting given that DDSI < 2.3 explicitly requires that all
HEARTBEAT messages sent by a writer advertise the existence of at least
1 sample, but this has been fixed in DDSI 2.3. As this requirement was
never respected by most DDSI implementations, there is no point in
retaining the setting, while it does remove a rather tricky problem
immediately after writer startup involving the conjuring up of a
sample that was annihilated immediately before it could have been
observed.
That conjuring up (as it turns out) can cause a malformed message to go
out (one that is harmless in itself). Fixing the generation of that
malformed message while the entire point of the trick is moot in DDSI
2.3 is a bit silly.
Note that full DDSI 2.3 compliance needs a bit more work, so not
bumping the DDSI protocol version number yet.
* Compatibility/AckNackNumbitsEmptySet: changing it from 0 breaks
compatibility with (at least) RTI Connext, and its reason for
existence disappers with a fix in DDSI 2.3.
* Internal/AggressiveKeepLastWhc: changing the setting from the default
made no sense whatsoever in Cyclone -- it would only add flow-control
and potentially block a keep-last writer where the spec forbids that.
* Internal/LegacyFragmentation: a left-over from almost a decade ago when
it was discovered that the specification was inconsistent in the use
of the message header flags for fragmented data, and this stack for a
while used the non-common interpretation. There is no reasonable way of
making the two modes compatible, and this setting merely existed to
deal with the compatibility issue with some ancient OpenSplice DDS
version.
* Durability/Encoding: historical junk.
* WatchDog and Lease: never had any function in Cyclone.
Signed-off-by: Erik Boasson <eb@ilities.com>
High sample rates require rather high rates of allocating and freeing
WHC nodes, serialised samples (serdata), and RTPS message fragments
(xmsg). A bunch of dedicated parallel allocators help take some
pressure off the regular malloc/free calls. However, these used to
gobble up memory like crazy, in part because of rather generous limits,
and in part because there was no restriction on the size of the samples
that would be cached, and it could end up caching large numbers of
multi-MB samples. It should be noted that there is no benefit to
caching large samples anyway, because the sample rate will be that much
lower.
This commit reduces the maximum number of entries for all three cases,
it furthermore limits the maximum size of a serdata or xmsg that can be
cached, and finally instead of instantiating a separate allocator for
WHC nodes per WHC, it now shares one across all WHCs. Total memory use
should now be limited to a couple of MB.
The caching can be disabled by setting ``FREELIST_TYPE`` to
``FREELIST_NONE`` in ``q_freelist.h``.
Signed-off-by: Erik Boasson <eb@ilities.com>
nn_bitset_one sets the specified number of bits by first memset'ing the
words, then clearing bits set in a final partial word. It mishandled
the case where the number of bits is a multiple of 32, clearing the
entire word following the last one it was to touch.
Signed-off-by: Erik Boasson <eb@ilities.com>
Generally one doesn't need to include any internal header files in an
application, but the (unstable) interface for application-defined sample
representation and serialization does require including some. It turns
out a keyword clash had to be resolved (typename => type_name) and that
a whole bunch of them were missing the #ifdef __cplusplus / extern "C"
bit.
It further turned out that one had to pull in nearly all of the type
definitions, including some typedefs that are illegal in C++, e.g.,
typedef struct os_sockWaitset *os_sockWaitset;
C++ is right to forbid this, but Cyclone's header files were wrong to
force inclusion of so much irrelevant stuff. This commit leaves these
typedefs in place, but eliminates a few header file inclusions to avoid
the problem.
Signed-off-by: Erik Boasson <eb@ilities.com>
Changes the semantics of dds_qget_{user,group,topic}data to always
append a 0 byte to any non-empty value without counting it in the size.
(An empty value is always represented by a null pointer and a size of
0). The advantage is that any code treating the data as the octet
sequence it formally is will do exactly the same, but any code written
with the knowledge that it should be a string can safely interpret it as
one.
Signed-off-by: Erik Boasson <eb@ilities.com>
As was the plan with the introduction of ddsrt; this includes renaming
the identifiers to match the capitalization style and removes old junk.
Signed-off-by: Erik Boasson <eb@ilities.com>
The built-in topics for readers and writers should be published before a
subscription or publication matched listener is invoked, otherwise the
instance handle provided to the listener is not yet available in a
reader for the corresponding topic.
Signed-off-by: Erik Boasson <eb@ilities.com>
Adds a new "ignorelocal" QoS to the readers/writers to ignore local
matching readers/writers, with three settings:
* DDS_IGNORELOCAL_NONE: default
* DDS_IGNORELOCAL_PARTICIPANT: ignores readers/writers in the same
participant
* DDS_IGNORELOCAL_PROCESS: ignores readers/writers in the same process
These can be set/got using dds_qset_ignorelocal and
dds_qget_ignorelocal.
If a matching reader or writer is ignored because of this setting, it is
as-if that reader or writer doesn't exist. No traffic will be generated
or data retained on its behalf.
There are no consequences for interoperability as this is (by
definition) a local affair.
Signed-off-by: Erik Boasson <eb@ilities.com>
The DDSI reader/writer pointers are now returned as out parameters
instead of as a return value, so that the upper-layer reference is set
before any listener can be invoked.
Signed-off-by: Erik Boasson <eb@ilities.com>
It was called strangely early in the deleting of the reader, even before
the DDSI reader was no longer being accessed by other threads. The
immediate and obvious problem is that it resets the pointer to the
upper-layer entity even though this can still be dereferenced in
invoking a listener, resulting in a crash.
Secondly it blocks until there are no listener calls any more (and the
resetting of that pointer will prevent any further listener
invocations), but a similar piece of logic is already in generic entity
code that resets the mask and then waits for all listener invocations to
complete. Having both is a problem.
Signed-off-by: Erik Boasson <eb@ilities.com>
There appears to be a minor performance benefit to not waking up the
delivery thread (if used) immediately upon enqueueing the first sample,
but rather to wait (typically) until the end of the packet. In a
latency measurement it probably makes little difference: one shouldn't
use asynchronous delivery if one needs the lowest possible latency, and
the end of the packet is reached rather quickly normally.
Signed-off-by: Erik Boasson <eb@ilities.com>
Remove all the "if asleep then awake ..." stuff from the code by making
awake/asleep calls nestable, whereas before it "awake ; awake" really
meant a transition through "asleep". This self-evidently necessitates
fixing those places where the old behaviour was relied on upon, but
fortunately those are few.
Signed-off-by: Erik Boasson <eb@ilities.com>
The old entity handle mechanism suffered from a number of problems, the
most terrible one being that it would only ever allocate 1000 handles
(not even have at most 1000 in use at the same time). Secondarily, it
was protected by a single mutex that actually does show up as a limiting
factor in, say, a polling-based throughput test with small messages.
Thirdly, it tried to provide for various use cases that don't exist in
practice but add complexity and overhead.
This commit totally rewrites the mechanism, by replacing the old array
with a hash table and allowing a near-arbitrary number of handles as
well as reuse of handles. It also removes the entity "kind" bits in the
most significant bits of the handles, because they only resulted in
incorrect checking of argument validity. All that is taken out, but
there is still more cleaning up to be done. It furthermore removes an
indirection in the handle-to-entity lookup by embedding the
"dds_handle_link" structure in the entity.
Handle allocation is randomized to avoid the have a high probability of
quickly finding an available handle (the total number of handles is
limited to a number much smaller than the domain from which they are
allocated). The likelihood of handle reuse is still dependent on the
number of allocated handles -- the fewer handles there are, the longer
the expected time to reuse. Non-randomized handles would give a few
guarantees more, though.
It moreover moves the code from the "util" to the "core/ddsc" component,
because it really is only used for entities, and besides the new
implementation relies on the deferred freeing (a.k.a. garbage collection
mechanism) implemented in the core.
The actual handle management has two variants, selectable with a macro:
the preferred embodiment uses a concurrent hash table, the actually used
one performs all operations inside a single mutex and uses a
non-concurrent version of the hash table. The reason the
less-predeferred embodiment is used is that the concurrent version
requires the freeing of entity objects to be deferred (much like the
GUID-to-entity hash tables in DDSI function, or indeed the key value to
instance handle mapping). That is a fair bit of work, and the
non-concurrent version is a reasonable intermediate step.
Signed-off-by: Erik Boasson <eb@ilities.com>
Fix the trace to contain a print of the address of the tkamp_instance
(along with the instance id), rather than the address of the stack
variable pointing to the tkmap_instance.
Signed-off-by: Erik Boasson <eb@ilities.com>
Some of the former are required to implement alternative serialisation
methods; the latter is just generally useful. For the time being these
are not part of the formal API and not subject to backwards
compatibility. Still, they have value for quickly building tools on that
use Cyclone and happen to need any of these functions.
Signed-off-by: Erik Boasson <eb@ilities.com>
Adding and removing reader/writer matches can be done by multiple
threads, and this can result in two threads simultaneously trying to do
this on a single reader/writer pair. The code therefore always checks
first whether the pair is (not) matched before proceeding.
However, removing a reader from a proxy writer had part of the code
outside this check. Therefore, if both entities are being deleted
simultanously, there is a risk that local_reader_ary_remove is called
twice for the same argument, and in that case, it asserts in one of them
because the reader can no longer be found. The counting of the number
of matched reliable readers suffers from the same race condition.
This commit eliminates these race conditions by moving these operations
into the block guarded by the aforementioned check.
Signed-off-by: Erik Boasson <eb@ilities.com>
When data arrives before a heartbeat has been received, it is impossible
to know whether this is a new "live" sample or a retransmit, and for
this reason the requesting of historical data is delayed until a
heartbeat arrives that informs the readers of the range of sequence
numbers to request as historical data.
However, by this time, and without this new condition in place, the
reader may have already received some data directly, and may
consequently request some data twice. That's not right.
Requiring a heartbeat to have been received before delivering the data
avoids this problem, but potentially delays receiving data after a new
writer/reader pair has been matched. The delay caused by a full
handshake at that point seems less bad that the odd case of stuttering
where that isn't expected. There are almost certainly some tricks
possible to avoid that delay in the common cases, but there are more
important things to do ...
Best-effort readers on a reliable proxy writer are a bit special: if
there are only best-effort readers, there is no guarantee that a
heartbeat will be received, and so the condition does not apply. This
commit attempts to deal with that by only requiring a heartbeat if some
reliable readers exist, but that doesn't allow a smooth transition from
"only best-effort readers" to "some reliable readers".
One could moreover argue that this condition should not be imposed on
volatile readers (at worst you get a little bit of data from before the
match), but equally well that it should (there's no guarantee that no
sample would be skipped in the case of a keep-all writer, if the first
sample happened to be a retransmit).
Signed-off-by: Erik Boasson <eb@ilities.com>
For compatibility with TwinOaks CoreDX, ignore an all-zero durability
service QoS received over SEDP for volatile and transient-local
endpoints.
Signed-off-by: Erik Boasson <eb@ilities.com>
The introduction of multiple receive threads could trigger the assertion
because a set of samples ready for delivery may have been received by
multiple threads (the problem manifests itself most easily with
fragmented samples). This is actually a non-issue:
* while synchronously processing a packet, there is a bias of 2**31
added to the refcount, to prevent any thread under any circumstance
from ever freeing the data;
* while data lives in the defragment buffers or reorder buffer of the
proxy writer, a bias of 2**20 is added to it until this particular
function is called, after delivery of the data to the readers, and
(if needed) after inserting the samples in the reorder buffer of
any readers that are out-of-sync with the proxy writer;
* the relevant refcount is updated atomically in such a manner that this
particular operation atomically removes the bias and performs the
delayed increment of the refcount to account for the data being stored
in any of the defragmenting or reorder buffers;
* the only ordinary decrementing of the refcount happens either
synchronously (if synchronous delivery is chosen), or asynchronously
in a delivery queue thread, and so the entire mechanism exists to
avoid premature freeing of the underlying data because the data is
delivered very quickly (possibly synchronously);
* as the biases are removed after all the delayed refcount increments
are taken into account and there are no increments following the call
to rmbias_and_adjust, the "ordinary" decrements can do no harm.
* the case of data from multiple writers being combined in a single
packet is dealt with by the 2**20 bias, and so there is potentially a
problem if there are more than 2**20 out-of-sync readers attached to
a single proxy writer, or data submessages from more than 2**11
writers in a single packet. The minimum possible data message is 32
bytes (headers, encoding, data, padding), so packets up to 64kB are
safe.
None of this is in any way related to which threads originally accepted
the packets, and therefore I see no argument for the existence of the
assertion.
That said, it is a rather complicated mechanism of unknown benefit, and
a major simplification is definitely something to be considered. In UDP
mode I see no chance of abuse, but there may be network protocols (TCP,
for sure) where there might be packets larger than 64kB and those could,
under worst-case assumptions, cause trouble. That, too, is a reason to
rethink it.
The call to rmbias_and_adjust was sometimes called with the proxy writer
locked, and sometimes after unlocking it. This commit changes it to
consistently call it with the lock held.
Signed-off-by: Erik Boasson <eb@ilities.com>
Sizing/ReceiveBufferSize must be >= Sizing/ReceiveBufferChunkSize + N
for some small N, and if it is not, Cyclone will crash reading beyond
allocated memory in a nasty way. Ordinarily this should be handled by
the configuration validation, but that would put the burden of knowing
the details of computing N upon the user, an unreasonable requirement.
The old state of an assertion presupposes a check, and brings us back
that same requirement.
Thus, a change to ensure that ReceiveBufferSize will be taken as the
minimum of the configured value and the actual minimal value as
determined by ChunkSize and whatever N happens to be.
Signed-off-by: Erik Boasson <eb@ilities.com>
During shutdown, the optional "debmon" thread for getting some
information about internal state of the DDSI stack had a tendency to run
into errors from calling write on a connection that had already been
closed immediately after connecting successfully to wake the thread.
Instead of blindly writing into the connection, it now checks whether it
is supposed to shutdown before doing anything, avoiding this particular
problem.
Signed-off-by: Erik Boasson <eb@ilities.com>
The rewrite of the abstraction layer changed some details in thread ids
used in tracing and functions to get those ids, with a result of always
printing the parent thread's id in create_thread rather than the newly
create thread's id. As all supported platforms use thread names in the
trace, it is a rather insignificant matter, and so this provides the
trivial fix by letting the new thread log the message.
Signed-off-by: Erik Boasson <eb@ilities.com>
The rtps_init function used to initialize all data structures and start
all threads used by the protocol stack, allowing discovery of remote
entities before the built-in topic data structures were initialized.
(Very) early discovery of a remote participant thus led to a crash.
This commit splits the initialisation, providing a separate function for
starting, in particular, the threads receiving data from the network.
In terms of threads created, it matches exactly with the rtps_stop /
rtps_fini split that already existed to address the exact same problem
on termination.
Signed-off-by: Erik Boasson <eb@ilities.com>
This adds an Internal/EnableExpensiveChecks setting for enabling some or all expensive run-time checks to avoid a massive slowdown when assertions are enabled at compile-time. Currently these cover only the writer and reader-history cache checking.
Signed-off-by: Erik Boasson <eb@ilities.com>
The configuration handling already allowed specifying multiple files in CYCLONEDDS_URI to be read in-order, this extends the behaviour to also allow the contents of these files to be embedded. This makes it possible to set a configuration without requiring a file system, or to add some ad-hoc options.
Signed-off-by: Erik Boasson <eb@ilities.com>
- Replace os_result by dds_retcode_t and move DDS return code defines down.
Eliminates the need to convert between different return code types.
- Move dds_time_t down and remove os_time.
Eliminates the need to convert between different time representations and
reduces code duplication.
- Remove use of Microsoft source-code annotation language (SAL).
SAL annotations are Microsoft specific and not very well documented. This
makes it very difficult for contributers to write.
- Rearrange the abstraction layer to be feature-based. The previous layout
falsely assumed that the operating system dictates which implementation is
best suited. For general purpose operating systems this is mostly true, but
embedded targets require a slightly different approach and may not even offer
all features. The new layout makes it possible to mix-and-match feature
implementations and allows for features to not be implemented at all.
- Replace the os prefix by ddsrt to avoid name collisions.
- Remove various portions of unused and unwanted code.
- Export thread names on all supported platforms.
- Return native thread identifier on POSIX compatible platforms.
- Add timed wait for condition variables that takes an absolute time.
- Remove system abstraction for errno. The os_getErrno and os_setErrno were
incorrect. Functions that might fail now simply return a DDS return code
instead.
- Remove thread-specific memory abstraction. os_threadMemGet and accompanying
functions were a mess and their use has been eliminated by other changes in
this commit.
- Replace attribute (re)defines by ddsrt_ prefixed equivalents to avoid name
collisions and problems with faulty __nonnull__ attributes.
Signed-off-by: Jeroen Koekkoek <jeroen@koekkoek.nl>
A writer blocking on a full WHC will still send out whatever it has
buffered but not sent yet. For this, the writer lock must be released,
but that means an ACK can sneak in between sending out the packet and
relocking the writer (not likely if there's a real network in between,
but over a loopback interface it is definitely possible).
Therefore, the amount of unacknowledged data that controls the blocking
and triggering of it must be refreshed before deciding to block,
otherwise it may hang indefinitely.
Signed-off-by: Erik Boasson <eb@ilities.com>
The writer tracks whether it is throttled because of a full WHC, but
does so by treating it as a simple flag. This is fine if there is at
most one thread blocked on any single writer at any time, but if there
are multiple threads using the same writer it would be possible for one
thread to be woken up, clear the flag, and so affect the wakeup of other
threads.
Turning it from a flag to a counter avoids that problem.
Signed-off-by: Erik Boasson <eb@ilities.com>
