eclipse-iceoryx / eclipse-iceoryx/iceoryx
Formalize non-shared-memory request-response communication with RouDi
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 492
- Avg merge
- 18h 57m
- Merged PRs (30d)
- 1
Description
## Brief feature description
Applications use a unix domain socket to register at RouDi and request ports. Currently this is done via a serialization to `std::string` and only the header is somewhat enforced but the payload with the commands is currently higgledy-piggledy. Some commands serialize one struct other multiple PODs and some combine both. It is cumbersome to add further parameter to a command.
This should be formalized and a common serialization should be used for all commands.
## Detailed information
This is a suggestion for a protocol from #332 which wanted to introduce a session Id to check for races when a message queue was deleted from the client and opened from a RouDi extension. This lead to a bug where the RouDi extension worked on obsolete data.
The unix domain socket communication needs a more sophisticated protocol with an header and payload. Since the communication can break in several ways, I would propose the following changes.
Step 1
- IpcMessage is split into header and payload, where the header is just a fixed, reserved amount of space at the beginning of the internal string and filled in sendMessageToProcess with the session ID which is passed alongside the IpcMessage
- to be able to create a more sophisticated header in the next steps, the first byte is the magic number 0xFF
- followed by 8 byte session id
- followed by two bytes payload size and the actual payload, which consists of the current protocol
- to keep our tests running, we need an "always valid" session id. this could be either 0 or -1u. this is not great for security reasons, but doesn't make it worse than the current situation
- framing
- magic number (0xFF): 1 byte -> maybe have this as protocol version byte and also use it later
- session id (uint64_t): 8 byte
- payload length (uint16_t, N): 2 bytes
- payload: N bytes
Step 2
- split IpcMessageType into several enums, since it contains to much unrelated stuff
- IpcMessageType (uint8_t)
- REQUEST-> for e.g. IMPL_SENDER
- RESPONSE -> for e.g. IMPL_SENDER
- NOTIFICATION -> better name???; for e.g. KEEP_ALIVE, WAKEUP_TRIGGER
- IpcMessageCommand (uint8_t)
- REG
- IMPL_SENDER
- FIND_SERVICE --> this could actually allocate a chunk from the mempool and just send a ChunkManagement pointer to the application
- ...
- INVALID -> this is the last one, used for sanity checks and unifies the current BEGIN, NO_TYPE and END enum values
- IpcMessageCommandResult (uint8_t)
- ACK
- ERROR
- IpcMessageErrorType (uint8_t)
- UNKNOWN_MESSAGE
- MESSAGE_NOT_SUPPORTED
- INCOMPATIBLE_PROTOCOL -> REG should send an expected protocol version, since we don't promise any binary compatibility
- NO_UNIQUE_CREATED
- SENDER_LIST_FULL
- INVALID -> used for sanity checks
- IpcMessageNotifications (uint8_t)
- KEEP_ALIVE -> this could be refactored to an atomic_flag; atomic_flag::clear() in app; atomic_flag::test_and_set() in RouDi; we need some memory for each application in the shared memory, though
- WAKEUP_TRIGGER -> this could be refactored to be triggered by a semaphore in the shared memory
- APP_WAIT -> this should probably remain a mqueue message; alternatively a multi pusher single popper queue in shm
- INVALID -> used for sanity checks
- framing for REQUEST/NOTIFICATION
- message type (uint8_t): 1 byte
- message command/notification (uint8_t): 1 byte
- session id (uint64_t): 8 byte
- payload size (uint16_t, N): 2 byte
- payload (like now, except IpcMessageType): N bytes
- framing for RESPONSE
- message type (uint8_t): 1 byte
- message command (uint8_t): 1 byte
- message command result (uint8_t): 1 byte
- session id (uint64_t): 8 byte
- payload size (uint16_t, N): 2 byte
- payload (like now, except IpcMessageType): N bytes
Step 3
- don't convert to ASCII, but introduce a binary protocol for the payload
- each payload entry consists of 2 bytes data length + actual data
- framing
- message type (uint8_t): 1 byte
- message command/notification (uint8_t): 1 byte
- session id (uint64_t): 8 byte
- payload size (uint16_t, N): 2 byte
- data entry size (uint16_t, X): 2 byte
- data entry: X byte
- data entry size (uint16_t, Y): 2 byte
- data entry: Y byte
Step 1 is necessary to fix this bug.
Step 2 will prevent us from bugs where the app receives interleaving messages. We already had this with the introduction of SERVICE_REGISTRY_CHANGE_COUNTER. With the NOTIFICATION approach, app requests could be cached in the app and the app could wait for the actual response.
Step 3 is not much work when Step 2 is done and will increase the performance since the serialization is now much simpler.
For Step 2 and 3 probably an own issue should be created. Maybe it's also unnecessary if we do the transition to a shm based communication, but even there we need some kind of protocol.
This could be done in combination with #1148
Contributor guide
Assessment
This issue has not been assessed yet.