Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Entity enable #530

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Entity enable #530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 57 additions & 29 deletions src/core/ddsc/include/dds/dds.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,46 +224,50 @@ dds_builtintopic_endpoint_t;
*/

/**
* @brief Enable entity.
* @brief Enable an entity.
*
* @note Delayed entity enabling is not supported yet (CHAM-96).
* Entities can be created in an enabled or disabled state.
* Enabled entities are immediately activated at creation time, and
* their immutable QoS settings cannot be changed.
* Disabled entities are not yet activated at creation time,
* so it is still possible to change their immutable QoS settings.
*
* This operation enables the dds_entity_t. Created dds_entity_t objects can start in
* either an enabled or disabled state. This is controlled by the value of the
* entityfactory policy on the corresponding parent entity for the given
* entity. Enabled entities are immediately activated at creation time meaning
* all their immutable QoS settings can no longer be changed. Disabled Entities are not
* yet activated, so it is still possible to change their immutable QoS settings. However,
* once activated the immutable QoS settings can no longer be changed.
* Creating disabled entities can make sense when the creator of the DDS_Entity
* does not yet know which QoS settings to apply, thus allowing another piece of code
* to set the QoS later on.
*
* The default setting of DDS_EntityFactoryQosPolicy is such that, by default,
* entities are created in an enabled state so that it is not necessary to explicitly call
* dds_enable on newly-created entities.
*
* The dds_enable operation produces the same results no matter how
* many times it is performed. Calling dds_enable on an already
* enabled DDS_Entity returns DDS_RETCODE_OK and has no effect.
* Typically, subscribers, publishers, readers and writers can be created in a disabled state.
* This is controlled by the value of the DDS_EntityFactoryQosPolicy of the parent
* of the entity. The default setting of DDS_EntityFactoryQosPolicy is such that
* entities are created in an enabled state.
*
* If an Entity has not yet been enabled, the only operations that can be invoked
* on it are: the ones to set, get or copy the QosPolicy settings, the ones that set
* (or get) the Listener, the ones that get the Status and the dds_get_status_changes
* operation (although the status of a disabled entity never changes). Other operations
* will return the error DDS_RETCODE_NOT_ENABLED.
*
* Entities created with a parent that is disabled, are created disabled regardless of
* the setting of the entityfactory policy.
*
* If the entityfactory policy has autoenable_created_entities
* set to TRUE, the dds_enable operation on the parent will
* automatically enable all child entities created with the parent.
*
* The Listeners associated with an Entity are not called until the
* Entity is enabled. Conditions associated with an Entity that
* is not enabled are "inactive", that is, have a trigger_value which is FALSE.
*
* Entities created with a parent that is disabled, are created disabled regardless of
* the setting of the DDS_EntityFactoryQosPolicy.
*
* Calling dds_enable() on a disabled DDS_Entity enables the DDS_Entity only if its
* parent entity is enabled. When the DDS Entity is enabled, the dds_enable() operation
* will also automatically enable all child entities of the DDS_Entity that have been
* created when the DDS_EntityFactoryQosPolicy of the DDS_Entity has
* autoenable_created_entities set to TRUE. For example, if a disabled subscriber
* which has its autoenable_created_entities set to TRUE is used to create 3 readers,
* then these 3 readers will be enabled automatically as soon as the subscriber is
* enabled.
*
* The dds_enable() operation will return DDS_RETCODE_OK if an entity and all its
* autoenabled children have been enabled successfully. In case one or more of
* its autoenabled children could not be enabled, an error code is returned.
* It is the responsibility of the application to find out which of the children
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really a fan of this model — I'd rather see no change at all when the operation fails — but considering that it is a relatively rarely used operation and that I imagine failure will usually be treated as a grounds for program termination, I think this is fine for a first iteration.

* code were enabled, and which not enabled.
*
* Calling dds_enable() on an already enabled DDS_Entity returns DDS_RETCODE_OK and
* has no effect.
*
* @param[in] entity The entity to enable.
*
* @returns A dds_return_t indicating success or failure.
Expand All @@ -279,6 +283,8 @@ dds_builtintopic_endpoint_t;
* The entity has already been deleted.
* @retval DDS_RETCODE_PRECONDITION_NOT_MET
* The parent of the given Entity is not enabled.
* @retval DDS_RETCODE_NOT_ALLOWED_BY_SECURITY
* The security plugin prevents enabling of the Entity.
*/
DDS_EXPORT dds_return_t
dds_enable(dds_entity_t entity);
Expand Down Expand Up @@ -1521,6 +1527,16 @@ dds_wait_for_acks(dds_entity_t publisher_or_writer, dds_duration_t timeout);
* A valid reader handle.
* @retval DDS_RETCODE_ERROR
* An internal error occurred.
* @retval DDS_RETCODE_ILLEGAL_OPERATION
* The provided entity is not a participant or subscriber.
* @retval DDS_RETCODE_BAD_PARAMETER
* One of the given arguments is not valid.
* @retval DDS_RETCODE_INCONSISTENT_POLICY
* The provided qos is not allowed.
* @retval DDS_RETCODE_NOT_ALLOWED_BY_SECURITY
* The security plugin prevents creation of the reader.
* @retval DDS_RETCODE_OUT_OF_RESOURCES
* There are not enough resources available to create the reader.
*/
/* TODO: Complete list of error codes */
DDS_EXPORT dds_entity_t
Expand Down Expand Up @@ -1593,9 +1609,21 @@ dds_reader_wait_for_historical_data(
* @returns A valid writer handle or an error code.
*
* @returns >0
* A valid writer handle.
* A valid writer handle.
* @returns DDS_RETCODE_ERROR
* An internal error occurred.
* An internal error occurred.
* @retval DDS_RETCODE_ERROR
* An internal error occurred.
* @retval DDS_RETCODE_ILLEGAL_OPERATION
* The provided entity is not a participant or subscriber.
* @retval DDS_RETCODE_BAD_PARAMETER
* One of the given arguments is not valid.
* @retval DDS_RETCODE_INCONSISTENT_POLICY
* The provided qos is not allowed.
* @retval DDS_RETCODE_NOT_ALLOWED_BY_SECURITY
* The security plugin prevents creation of the writer.
* @retval DDS_RETCODE_OUT_OF_RESOURCES
* There are not enough resources available to create the writer.
*/
/* TODO: Complete list of error codes */
DDS_EXPORT dds_entity_t
Expand Down
23 changes: 23 additions & 0 deletions src/core/ddsc/include/dds/ddsc/dds_public_qos.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,17 @@ dds_qunset_bprop (
dds_qos_t * __restrict qos,
const char * name);

/* @brief Set the entity-factory policy of a qos structure
*
* @param[in,out] qos - Pointer to a dds_qos_t structure that will store the policy
* @param[in] autoenable - True iff entities that are created from the factory for which
* this qos is applied are enabled
*/
DDS_EXPORT void
dds_qset_entity_factory (
dds_qos_t * __restrict qos,
bool autoenable);

/**
* @brief Set the type consistency enforcement policy of a qos structure
*
Expand Down Expand Up @@ -866,6 +877,18 @@ dds_qget_type_consistency (
bool *prevent_type_widening,
bool *force_type_validation);

/* @brief Get the entity-factory qos policy
*
* @param[in] qos - Pointer to a dds_qos_t structure storing the policy
* @param[in,out] autoenable - Pointer that will store whether to enable child entities when created
*
* @returns - false iff any of the arguments is invalid or the requested qos value is not
* present in the qos object
*/
DDS_EXPORT bool dds_qget_entity_factory (
const dds_qos_t * __restrict qos,
bool *autoenable);

#if defined (__cplusplus)
}
#endif
Expand Down
7 changes: 6 additions & 1 deletion src/core/ddsc/src/dds__entity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
extern "C" {
#endif

DDS_EXPORT dds_return_t dds_entity_autoenable_children (dds_entity *entity);

DDS_EXPORT dds_entity_t
dds_entity_init(
dds_entity * e,
Expand Down Expand Up @@ -68,7 +70,7 @@ DDS_EXPORT inline dds_entity *dds_entity_from_handle_link (struct dds_handle_lin
}

DDS_EXPORT inline bool dds_entity_is_enabled (const dds_entity *e) {
return (e->m_flags & DDS_ENTITY_ENABLED) != 0;
return ((e->m_flags & DDS_ENTITY_ENABLED) != 0);
}

DDS_EXPORT void dds_entity_status_set (dds_entity *e, status_mask_t t);
Expand Down Expand Up @@ -176,6 +178,9 @@ dds_generic_unimplemented_operation(
dds_entity_t handle,
dds_entity_kind_t kind);

DDS_EXPORT bool
dds_entity_creation_allowed (
const dds_return_t rc);

#if defined (__cplusplus)
}
Expand Down
11 changes: 9 additions & 2 deletions src/core/ddsc/src/dds__types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,10 @@ struct dds_listener {

/* Entity flag values */

#define DDS_ENTITY_ENABLED ((uint32_t) 0x1) /* DDS "enabled" state */
#define DDS_ENTITY_IMPLICIT ((uint32_t) 0x2) /* implicit ones get deleted when the last child is deleted */
#define DDS_ENTITY_ENABLED ((uint32_t) 0x1) /* DDS "enabled" state */
#define DDS_ENTITY_IMPLICIT ((uint32_t) 0x2) /* implicit ones get deleted when the last child is deleted */
#define DDS_ENTITY_ENABLE_ON_PARENT ((uint32_t) 0x4) /* flag that indicates that the entity was created by a parent
with autoenable=true while the parent was disabled */

struct dds_domain;
struct dds_entity;
Expand All @@ -108,6 +110,7 @@ typedef struct dds_entity_deriver {
dds_return_t (*validate_status) (uint32_t mask);
struct dds_statistics * (*create_statistics) (const struct dds_entity *e);
void (*refresh_statistics) (const struct dds_entity *e, struct dds_statistics *s);
dds_return_t (*enable) (struct dds_entity *e) ddsrt_nonnull_all;
} dds_entity_deriver;

struct dds_waitset;
Expand Down Expand Up @@ -182,6 +185,7 @@ dds_return_t dds_entity_deriver_dummy_set_qos (struct dds_entity *e, const dds_q
dds_return_t dds_entity_deriver_dummy_validate_status (uint32_t mask);
struct dds_statistics *dds_entity_deriver_dummy_create_statistics (const struct dds_entity *e);
void dds_entity_deriver_dummy_refresh_statistics (const struct dds_entity *e, struct dds_statistics *s);
dds_return_t dds_entity_deriver_dummy_enable (struct dds_entity *e);

inline void dds_entity_deriver_interrupt (struct dds_entity *e) {
(dds_entity_deriver_table[e->m_kind]->interrupt) (e);
Expand Down Expand Up @@ -210,6 +214,9 @@ inline struct dds_statistics *dds_entity_deriver_create_statistics (const struct
inline void dds_entity_deriver_refresh_statistics (const struct dds_entity *e, struct dds_statistics *s) {
dds_entity_deriver_table[e->m_kind]->refresh_statistics (e, s);
}
inline dds_return_t dds_entity_deriver_enable (struct dds_entity *e) {
return dds_entity_deriver_table[e->m_kind]->enable (e);
}

typedef struct dds_cyclonedds_entity {
struct dds_entity m_entity;
Expand Down
18 changes: 18 additions & 0 deletions src/core/ddsc/src/dds_builtin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,23 @@ static struct ddsi_tkmap_instance *dds__builtin_get_tkmap_entry (const struct dd
return tk;
}

static struct ddsi_tkmap_instance *dds__builtin_get_tkmap_entry_with_iid (const struct ddsi_guid *guid, const uint64_t iid, void *vdomain)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function serves a rather unique purpose: new local entities now need to have an iid before being enabled, and in the way it is implemented in this PR before the GUID is assigned. In all other cases the iid gets assigned on mapping the key to an instance id, and for entities that GUID is the key.

Technically, the possibility exists that a GUID gets reused, there are after all but 2^24 entity ids (in Cyclone, arguably different entity kinds could use the same entity id). So a process that creates 2^24 entities (but for some rounding errors), deletes one of them (so there is again an entity id available), and then creates another entity will necessarily reuse the entity id of the entity that was just deleted. However the GUID-to-iid mapping may outlive the entity that was deleted.

Consequently, in the design in this PR, come the call dds__builtin_get_tkmap_entry_with_iid on the creation of the second entity with GUID guid, if the old GUID-to-iid mapping for that GUID still exists, you end up in a state where the mapping retains the old iid, whereas the entity object has the new iid. That'll lead to an observable difference (somewhere, I can't be bothered to figure out all the details). I am sure you'll agree this is an edge case, but edge cases are usually more interesting than regular ones 🙂.

Methinks the solution lies in not introducing this new path, but rather moving the GUID allocation (e.g., in new_reader) up in the stack (e.g., to dds_create_reader, or perhaps even to dds_entity_init) so that the GUID-to-iid mapping can always follow the same path and the potential discrepancy avoided.

(This has various effects throughout the PR, I'm not going to mark them, that's not worth the bother.)

{
struct dds_domain *domain = vdomain;
struct ddsi_tkmap_instance *tk;
struct ddsi_serdata *sd;
union { ddsi_guid_t guid; struct ddsi_keyhash keyhash; } x;
x.guid = nn_hton_guid (*guid);
/* any random builtin topic will do (provided it has a GUID for a key), because what matters is the "class"
of the topic, not the actual topic; also, this is called early in the initialisation of the entity with
this GUID, which simply causes serdata_from_keyhash to create a key-only serdata because the key lookup
fails. */
sd = ddsi_serdata_from_keyhash (domain->builtin_participant_type, &x.keyhash);
tk = ddsi_tkmap_find_with_iid (domain->gv.m_tkmap, sd, iid, true);
ddsi_serdata_unref (sd);
return tk;
}

struct ddsi_serdata *dds__builtin_make_sample (const struct entity_common *e, ddsrt_wctime_t timestamp, bool alive)
{
/* initialize to avoid gcc warning ultimately caused by C's horrible type system */
Expand Down Expand Up @@ -273,6 +290,7 @@ void dds__builtin_init (struct dds_domain *dom)

dom->btif.arg = dom;
dom->btif.builtintopic_get_tkmap_entry = dds__builtin_get_tkmap_entry;
dom->btif.builtintopic_get_tkmap_entry_with_iid = dds__builtin_get_tkmap_entry_with_iid;
dom->btif.builtintopic_is_builtintopic = dds__builtin_is_builtintopic;
dom->btif.builtintopic_is_visible = dds__builtin_is_visible;
dom->btif.builtintopic_write = dds__builtin_write;
Expand Down
77 changes: 66 additions & 11 deletions src/core/ddsc/src/dds_entity.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ struct dds_statistics *dds_entity_deriver_dummy_create_statistics (const struct
void dds_entity_deriver_dummy_refresh_statistics (const struct dds_entity *e, struct dds_statistics *s) {
(void) e; (void) s;
}
dds_return_t dds_entity_deriver_dummy_enable (struct dds_entity *e) {
(void) e; return DDS_RETCODE_ILLEGAL_OPERATION;
}

extern inline void dds_entity_deriver_interrupt (struct dds_entity *e);
extern inline void dds_entity_deriver_close (struct dds_entity *e);
Expand All @@ -78,6 +81,7 @@ extern inline bool dds_entity_supports_set_qos (struct dds_entity *e);
extern inline bool dds_entity_supports_validate_status (struct dds_entity *e);
extern inline struct dds_statistics *dds_entity_deriver_create_statistics (const struct dds_entity *e);
extern inline void dds_entity_deriver_refresh_statistics (const struct dds_entity *e, struct dds_statistics *s);
extern inline dds_return_t dds_entity_deriver_enable (struct dds_entity *e);

static int compare_instance_handle (const void *va, const void *vb)
{
Expand Down Expand Up @@ -196,6 +200,46 @@ static bool entity_kind_has_qos (dds_entity_kind_t kind)
}
#endif

dds_return_t dds_entity_autoenable_children (dds_entity *e)
{
struct dds_entity *c;
ddsrt_avl_iter_t it;
dds_return_t rc = DDS_RETCODE_OK, ret;

assert(e);
assert(e->m_flags & DDS_ENTITY_ENABLED);

/* enable all disabled children with the flag DDS_ENTITY_ENABLE_ON_PARENT */
for (c = ddsrt_avl_iter_first (&dds_entity_children_td, &e->m_children, &it); c != NULL; c = ddsrt_avl_iter_next (&it)) {
if ((!(c->m_flags & DDS_ENTITY_ENABLED)) && (c->m_flags & DDS_ENTITY_ENABLE_ON_PARENT)) {
ret = dds_enable((dds_entity_t)c->m_hdllink.hdl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is invoked by the entity kind-specific function on e while e's entity lock is held, but that means it locks the children's entity locks while holding the parent's entity lock, which breaks the rules. Perhaps it would have been better to allow locking a child entity while holding the parent's lock, rather than the other way around, but today you can only easily walk "upward" in the hierarchy.

So here is a risk of deadlock, however small (there are rather few operations that would actually try to lock a disabled entity, but certainly calling dds_enable on a publisher and on its writers in parallel is one way.)

The trick to do such a walk over entities typically involves unlocking & relocking the parent entity. See, for example, pushdown_pubsub_qos.

if ((rc == DDS_RETCODE_OK) && (ret < 0)) {
rc = ret;
}
}
}
return rc;
}

static void dds_entity_init_mflags (dds_entity *e, dds_entity *parent, bool implicit)
{
assert (e);

if (implicit)
e->m_flags |= DDS_ENTITY_IMPLICIT;
/* entity is enabled by default unless parent is not enabled or parent has
* autoenable_created_entities set to false */
e->m_flags |= DDS_ENTITY_ENABLED;
if (parent) {
if (parent->m_qos && parent->m_qos->entity_factory.autoenable_created_entities)
/* parent has autoenable set to true */
e->m_flags |= DDS_ENTITY_ENABLE_ON_PARENT;
/* do not enable if parent is not enabled or no autoenable */
if ((!(parent->m_flags & DDS_ENTITY_ENABLED)) || ((e->m_flags & DDS_ENTITY_ENABLE_ON_PARENT) == 0))
e->m_flags &= ~DDS_ENTITY_ENABLED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rewriting the logic to set ENABLED once if and only if it really is enabled is much clearer than setting it and clearing it if turns out that it shouldn't be set.

}
}

dds_entity_t dds_entity_init (dds_entity *e, dds_entity *parent, dds_entity_kind_t kind, bool implicit, dds_qos_t *qos, const dds_listener_t *listener, status_mask_t mask)
{
dds_handle_t handle;
Expand All @@ -211,10 +255,10 @@ dds_entity_t dds_entity_init (dds_entity *e, dds_entity *parent, dds_entity_kind
e->m_cb_pending_count = 0;
e->m_observers = NULL;

/* TODO: CHAM-96: Implement dynamic enabling of entity. */
e->m_flags |= DDS_ENTITY_ENABLED;
if (implicit)
e->m_flags |= DDS_ENTITY_IMPLICIT;
/* All entities are enabled by default, except if the parent is not enabled or
* if the parent's entity factory qos indicates that children of this parent
* should not be enabled when created */
dds_entity_init_mflags (e, parent, implicit);

/* set the status enable based on kind */
if (entity_has_status (e))
Expand Down Expand Up @@ -1059,15 +1103,11 @@ dds_return_t dds_enable (dds_entity_t entity)

if ((rc = dds_entity_lock (entity, DDS_KIND_DONTCARE, &e)) != DDS_RETCODE_OK)
return rc;

/* only enable entities that are not yet enabled */
if ((e->m_flags & DDS_ENTITY_ENABLED) == 0)
{
/* TODO: Really enable. */
e->m_flags |= DDS_ENTITY_ENABLED;
DDS_CERROR (&e->m_domain->gv.logconfig, "Delayed entity enabling is not supported\n");
}
rc = dds_entity_deriver_enable(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does make some sense (enabling is definitely specific to the entity kind), but it doesn't actually dispatch to an implementation for all entity kinds (e.g., what about topics? — and if those don't need any special handling, dds_entity_deriver_dummy_enable simply returns "illegal operation", so is a rather incomplete operation).

dds_entity_unlock (e);
return DDS_RETCODE_OK;
return rc;
}

dds_return_t dds_get_status_changes (dds_entity_t entity, uint32_t *status)
Expand Down Expand Up @@ -1327,10 +1367,15 @@ dds_return_t dds_triggered (dds_entity_t entity)

if ((ret = dds_entity_lock (entity, DDS_KIND_DONTCARE, &e)) != DDS_RETCODE_OK)
return ret;
if (!dds_entity_is_enabled(e)) {
ret = DDS_RETCODE_NOT_ENABLED;
goto err_enabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same amount of code as

dds_entity_unlock (e);
return DDS_RETCODE_NOT_ENABLED;

in other words, the use of a goto doesn't really add much. I think it'd be better to use my suggestion.

}
if (entity_has_status (e))
ret = ((ddsrt_atomic_ld32 (&e->m_status.m_status_and_mask) & SAM_STATUS_MASK) != 0);
else
ret = (ddsrt_atomic_ld32 (&e->m_status.m_trigger) != 0);
err_enabled:
dds_entity_unlock (e);
return ret;
}
Expand Down Expand Up @@ -1546,3 +1591,13 @@ dds_return_t dds_assert_liveliness (dds_entity_t entity)
dds_entity_unpin (e);
return rc;
}

bool dds_entity_creation_allowed (const dds_return_t rc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this function makes things clearer than simply inlining it at the two call sites.

{
switch (rc) {
case DDS_RETCODE_NOT_ALLOWED_BY_SECURITY:
return false;
default:
return true;
}
}
Loading