eclipse-iceoryx / eclipse-iceoryx/iceoryx

Formalize non-shared-memory request-response communication with RouDi

Open
#1,133 0 comments 0 reactions 1 assignee Claimed by @elBoberido View on GitHub
enhancement
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.