niklas/cyclonedds
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

a read/take returning no data must be a no-op

Browse source 
In all cases where read/take allocates memory for storing samples but
the result turns out to be an empty set, the (observable) state of the
system should end up unchanged.

It turns out several cases were/are considered:

* application supplies buffers (i.e., buf[0] != NULL): no memory
  allocated, so no issue.

* reader has no cached set ("m_loan" in the current code): read/take
  allocated memory, cached the address and marked it as in use
  ("m_loan_out"), and modified buf[0] (and subsequent entries).

  To undo this on returning an empty set, it now: resets the
  "m_loan_out" flag to allow the cached buffer to be reused, and sets
  buf[0] back to NULL.

* reader has a cached set, but it is not marked in use: read/take
  marked it as in use and modified buf[0] (and subsequent entries).

  To undo this, it now resets "m_loan_out" to indicate the cached buffer
  is not in use, and sets buf[0] back to NULL.

* reader has a cached set that is currently in use: read/take allocated
  memory and updated buf[0] (and subsequent entries) but left the cached
  state alone.

  To undo this, it now frees the memory and sets buf[0] back to NULL.

With this, in any path where the application lets dds_read/dds_take
allocate memory for the samples:

* it can still safely call dds_return_loan with buf[0] and the actual
  return value of read/take (even if an error code), and whatever memory
  was allocated will not be leaked;

* but it no longer has to do so when the result was empty (or error).

Signed-off-by: Erik Boasson <eb@ilities.com>
This commit is contained in:
Erik Boasson 2019-05-07 15:12:31 +08:00 • committed by eboasson
parent a6d92aac8c
commit d16264fd82
1 changed files with 31 additions and 8 deletions
Download patch file Download diff file Expand all files Collapse all files

39
src/core/ddsc/src/dds_read.c
View file

@ -94,6 +94,10 @@ dds_read_impl(
dds_retcode_t rc;
struct dds_reader * rd;
struct dds_readcond * cond;
unsigned nodata_cleanups = 0;
#define NC_CLEAR_LOAN_OUT 1u
#define NC_FREE_BUF 2u
#define NC_RESET_BUF 4u
if (buf == NULL) {
DDS_ERROR("The provided buffer is NULL\n");
@ -140,21 +144,25 @@ dds_read_impl(
/* Allocate, use or reallocate loan cached on reader */
if (rd->m_loan_out) {
ddsi_sertopic_realloc_samples (buf, rd->m_topic->m_stopic, NULL, 0, maxs);
nodata_cleanups = NC_FREE_BUF | NC_RESET_BUF;
} else {
if (rd->m_loan) {
if (rd->m_loan_size < maxs) {
ddsi_sertopic_realloc_samples (buf, rd->m_topic->m_stopic, rd->m_loan, rd->m_loan_size, maxs);
rd->m_loan = buf[0];
rd->m_loan_size = maxs;
nodata_cleanups = NC_RESET_BUF;
} else {
buf[0] = rd->m_loan;
buf[0] = rd->m_loan;
nodata_cleanups = NC_RESET_BUF;
}
} else {
ddsi_sertopic_realloc_samples (buf, rd->m_topic->m_stopic, NULL, 0, maxs);
rd->m_loan = buf[0];
rd->m_loan_size = maxs;
}
} else {
ddsi_sertopic_realloc_samples (buf, rd->m_topic->m_stopic, NULL, 0, maxs);
rd->m_loan = buf[0];
rd->m_loan_size = maxs;
}
rd->m_loan_out = true;
nodata_cleanups |= NC_CLEAR_LOAN_OUT;
}
}
@ -169,9 +177,24 @@ dds_read_impl(
ddsrt_mutex_unlock (&rd->m_entity.m_observers_lock);
if (take) {
ret = (dds_return_t)dds_rhc_take(rd->m_rd->rhc, lock, buf, si, maxs, mask, hand, cond);
ret = (dds_return_t) dds_rhc_take (rd->m_rd->rhc, lock, buf, si, maxs, mask, hand, cond);
} else {
ret = (dds_return_t)dds_rhc_read(rd->m_rd->rhc, lock, buf, si, maxs, mask, hand, cond);
ret = (dds_return_t) dds_rhc_read (rd->m_rd->rhc, lock, buf, si, maxs, mask, hand, cond);
}
/* if no data read, restore the state to what it was before the call, with the sole
exception of holding on to a buffer we just allocated and that is pointed to by
rd->m_loan */
if (ret <= 0 && nodata_cleanups) {
if (nodata_cleanups & NC_CLEAR_LOAN_OUT) {
rd->m_loan_out = false;
}
if (nodata_cleanups & NC_FREE_BUF) {
ddsi_sertopic_free_samples (rd->m_topic->m_stopic, buf[0], maxs, DDS_FREE_ALL);
}
if (nodata_cleanups & NC_RESET_BUF) {
buf[0] = NULL;
}
}
dds_read_unlock(rd, cond);