Redundant network, send queue triggering & some other fixes #1858
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Erik Boasson <[email protected]>
This one is always non-null (and it has even been dereferenced unconditionally earlier in the function). Signed-off-by: Erik Boasson <[email protected]>
The async send thread would only be triggered on the second RTPS message, so an application that publishes exactly one best-effort sample via the async path would not actually transmit anything. (Reliable data will be sent because of the reliable protocol triggering a retransmit.) Signed-off-by: Erik Boasson <[email protected]>
Signed-off-by: Erik Boasson <[email protected]>
Signed-off-by: Erik Boasson <[email protected]>
eboasson
force-pushed
the
fix-dst-all-uc
branch
2 times, most recently
from
2f15111 to
959fa70
Compare
dpotman
reviewed
Oct 23, 2023
src/core/ddsi/src/ddsi_wraddrset.c
Outdated
| (*c)->nreaders += chunk; | ||
| (*c) = ddsrt_realloc (*c, cover_size ((*c)->nreaders + chunk, (*c)->nlocs)); |
There was a problem hiding this comment.
Duplicate + chunk, because nreaders is updated on line 72
There was a problem hiding this comment.
Oops! Good catch. I decided to force-push it, it is such a trivial change and otherwise I would need to squash it afterward before merging ...
Redundant networking mode extends the matrix, the entries were left uninitialized, but they are used. The initial code simply allocated some more memory than needed to allow for a reasonable number of redundant readers, the fix that lifted that hard limit introduced the bug (522533c). Signed-off-by: Erik Boasson <[email protected]>
The particular destination mode is used for unicasts in redundant network mode, and so it is not supposed to send it also to multicast addresses. While setting to ALL_UC uses the "all_uc" union case and so sending it as-if set to ALL means it uses the wrong case, the damage is limited because the two cases have exactly the same content. It is only a performance/network abuse problem. Signed-off-by: Erik Boasson <[email protected]>
Originally the code base allowed for remote entiites that did not have a unicast address, or at least something classified as such. This was already (mostly) abandoned, but remnants remained (in the form of any_uc_else_mc). SPDP and SEDP already checked, so in principle none of those remnants was still necessary. The introduction of PSMX did change that a bit: the requirement is now that there must be at least non-PSMX unicast locator. This commit updates those checks and eliminates the paths that can't be reached as a consequence of improving the validation code. Signed-off-by: Erik Boasson <[email protected]>
This test constructs a reader and a writer connected via two networks, then verifies that the writer sends the data to both addresses and that the ACKNACK is send to two unicast addresses. That covers the basic mechanism as well as the fix for 7ad55db. It does so by inspecting the trace output. This is far from ideal, but there are not that many ways of finding out what IP addresses a packet actually gets sent to without interposing in the transports, letting it write a packet capture file or using tcpdump. All of which are more involved still. Signed-off-by: Erik Boasson <[email protected]>
Signed-off-by: Erik Boasson <[email protected]>
eboasson
force-pushed
the
fix-dst-all-uc
branch
from
959fa70 to
b3a883c
Compare
dpotman
approved these changes
Oct 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apologies for the varied collection of things in this PR ...
It also adds tests for them and fixes some things that were discovered in the process.