Confusion about multiple outgoing LTP peers

[BrianSipos]

Currently, the LtpUdpEngineManager::AddLtpUdpEngine() interface seems to have some logical issues when using one LTP engine to communicate with multiple peer engines. The inbound configuration separates "own engine ID" from "peer engine ID" but the outbound does not identify the target peer engine ID.

This seems to mean that a single LtpUdpEngineManager can only transmit blocks to a single peer engine, which means that each peer must be associated with a unique local UDP port. There is no logical reason why this restriction should be present, and I think that changing the signature from

AddLtpUdpEngine(const uint64_t thisEngineId, const uint64_t expectedSessionOriginatorEngineId, ...)

to

AddLtpUdpEngine(const uint64_t thisEngineId, const uint64_t remoteEngineId, ...)

would avoid this problem and allow each peer a separate LtpEngine outduct. This would then affect the internal logic and the logic for GetLtpUdpEnginePtr() and some other public functions, but I think these would all be improvements in usability.

[BrianSipos]

Using the session originator as the discriminator works fine for dispatching incoming segment datagrams for sessions originated from other engines, but for sessions originated by this engine the session ID is the unique discriminator to decide how to handle the datagram.

I think this means that there should be two internal maps:

1. For induct (RX) sessions, a map of originator to engine-object. Very similar to current behavior just removing the boolean indicator from the map key.
2. For transmitting sessions, a map of session-id to engine-object. This would be a completely separate map, and thanks to LTP design of all sessions being independent there is no need for any relation between the two engine maps.

[BrianSipos]

After doing some experimentation, the current engine-id-and-direction-bool map is still needed for local object management (allowing the Add... and Remove... functions to keep their signatures). Only the argument expectedSessionOriginatorEngineId changes its meaning to remoteEngineId. The second map is still needed when the originating engine is the local engine.

To allow the engine pointers in multiple containers, I changed the storage type in LtpUdpEngineManager from std::uniquq_ptr<LtpUdpEngine> to std::shared_ptr<LtpUdpEngine> but the ownership logic is still the same. It may actually be a little safer to have the public API in that class also return a shared_ptr to avoid the possibility of a dangling pointer... but that's an API safety issue not a "bad" current behavior.

[briantomko]

This issue has been resolved with the commit of 69-multiple-outgoing-ltp-peers into master

The API was changed to: AddLtpUdpEngine(const uint64_t thisEngineId, const uint64_t remoteEngineId, ...)

There are now separate maps in the LtpUdpEngineManager, replacing the boolean isInduct map.

For outduct (and only outduct) sessions, the AddLtpUdpEngine creates a unique "engine id index" between 1 and 255 which gets encoded into the most significant byte of the randomly generated LTP Session Number. When packets come back to the shared udp socket, the socket router (LtpUdpEngineManager::HandleUdpReceive) can know which LtpEngine to give the message to (REPORT_SEGMENT, CANCEL_ACK_SEGMENT_TO_BLOCK_SENDER, or CANCEL_SEGMENT_FROM_BLOCK_RECEIVER). Hence, there can be a maximum of 254 unique contacts per outduct udp port.

For Induct sessions, the behavior remains the same, using the sessionOriginatorEngineId which is also the remote engine id.