Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix event config loading defaulting to RT_UNRELIABLE #866

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TrevorTSN
Copy link

Summary:
When events are first parsed, the config loader reads eventgroups first. Because the event doesn't have an existing configuration, it creates one with RT_UNRELIABLE (UDP) by default. But when the actual event is parsed, it detects the dummy event and displays a VSOMEIP_INFO message; and the event reliability type is not being correctly updated to RT_RELIABLE (TCP). This fix changes the default reliability type to RT_UNKNOWN when events are created from 'eventgroup' parsing. And then updates the event correctly when the actual event entry is parsed.

@TrevorTSN
Copy link
Author

TrevorTSN commented Mar 10, 2025

For more context. In the v3.3 branch, there were significant (breaking/regression) changes made to parsing and handling of (temporary) 'event' entries being created when parsing "eventgroups" and then being updated when the event was actually parsed later.

For context, here is a small config file snippet:

            "eventgroups": [
                {
                    "eventgroup": "0xc001",
                    "events": ["0xc101"]
                }
            ],
            "events": [
                {
                    "event": "0xc101",
                    "is_reliable": "true"
                }
            ],

In the existing code, during config file parsing, "eventgroups" are processed before "events" (lexical order). The event "0xc101" is created with a dummy/placeholder/temporary event entry with a reliability type set to RT_UNRELIABLE (in the existing code), and should have been updated with it's actual reliability type set later when the "events" section is parsed.

In the v3.3 the following code was removed:

            if (found_event != _service->events_.end()) {
                 if (found_event->second->is_placeholder_) {
                     its_event = found_event->second;
                 } else {
                     VSOMEIP_ERROR << "Multiple configurations for event ["
                             << std::hex << _service->service_ << "."
                             << _service->instance_ << "."
                             << its_event_id << "]";
                 }
             } else {

and replaced with the following in v3.3:

            if (found_event != _service->events_.end()) {
                VSOMEIP_INFO << "Multiple configurations for event ["
                        << std::hex << _service->service_ << "."
                        << _service->instance_ << "."
                        << its_event_id << "].";
            } else {

As you can see, the is_placeholder_ check was removed, but an equivalent wan't created in the new code to determine if the event is a temporary/placeholder entry or not.

This is_placeholder_ value was being set, when a new event entry was created during parsing of "eventgroups". "eventgroups" are actually parsed before "events" are parsed, because the boost property_tree keys are an "ordered list", event(g)roups are lexically parsed before event(s).

So what happens currently, is that a "dummy"/"placeholder" event is created (because the eventgroup only knows the event ID, and not the details of the event yet); but the event is created with a default reliability type of reliability_type_e::RT_UNRELIABLE (UDP). However, we (our company) do NOT use UDP, we use TCP reliability_type_e::RT_RELIABLE, which is set in the "event" configuration section. But the existing code has no way to determine if the event entry created is a "dummy"/"placeholder" event created by the "eventgroups" section or not, so it skips updating the reliability type and other settings, and just displays a "VSOMEIP_INFO" message, which is NOT sufficient anyway (it should be VSOMEIP_WARNING).

This is a BREAKING regression issue for event parsing. Without a sufficient way to determine if a "placeholder" event was created or not, the existing code skips updating the reliability type of the event.

What I've done instead, is to use the RT_UNKNOWN reliability type as a "placeholder" value when the dummy entry is created when parsing the eventgroups entry. This will be detected and updated later when the ACTUAL event entry is parsed in the config file.

This is the existing code that creates a "dummy"/"placeholder" events entry. This code is found in the load_eventgroup function:

                    if (0 < its_event_id) {
                        std::shared_ptr<event> its_event(nullptr);
                        auto find_event = _service->events_.find(its_event_id);
                        if (find_event != _service->events_.end()) {
                            its_event = find_event->second;
                        } else {
                            its_event = std::make_shared<event>(its_event_id,
                                            false, reliability_type_e::RT_UNRELIABLE,
                                            std::chrono::milliseconds::zero(),
                                            false, true);
                        }
                        if (its_event) {
                            its_event->groups_.push_back(its_eventgroup);
                            its_eventgroup->events_.insert(its_event);
                            _service->events_[its_event_id] = its_event;
                        }
                    }

As you can see from the following lines:

                            its_event = std::make_shared<event>(its_event_id,
                                            false, reliability_type_e::RT_UNRELIABLE,
                                            std::chrono::milliseconds::zero(),
                                            false, true);

An RT_UNRELIABLE (UDP) entry is made while parsing events in the "eventgroups" section, however, the "eventgroup" cannot actually determine if it's an RT_UNRELIABLE or not. Defaulting to RT_UNRELIABLE is an issue if you have no way to update it later to RT_RELIABLE.

For reference, the OLD 3.2 code to create the temporary/placeholder entry in "events" just looked like this:

                            its_event = std::make_shared<event>(its_event_id);

Without the RT_UNRELIABLE type being set, and internally there was the is_placeholder_ flag which was true by default.
The new code (from v3.3) removed the placeholder flag and defaulted the entry to RT_UNRELIABLE. Then when the actual "event" is parsed, the new code doesn't update the reliabilty type.

Here is the diff between the old 3.2 and 3.3 branch, you'll need to expand the "vsomeip/implementation/configuration/src/configuration_impl.cpp" file on line 1655:
826ebb8#diff-59ac9614ab179eb92624f31fe73d36899f23a2e430f1e7dab6a8953df702aa5cL1655

@TrevorTSN TrevorTSN force-pushed the fix_event_config_loading branch from 04e3ff5 to a03310f Compare March 10, 2025 09:32
Summary:
When events are first parsed, the config loader reads eventgroups first.
Because the event doesn't have an existing configuration, it creates one with RT_UNRELIABLE by default.
But when the actual event is parsed, it detects the dummy event and displays a VSOMEIP_INFO message;
and the event reliability type is not being correctly updated to RT_RELIABLE.
This fix changes the default reliability type to RT_UNKNOWN when events are created from 'eventgroup' parsing.
And then updates the event correctly when the actual event entry is parsed.
@TrevorTSN TrevorTSN force-pushed the fix_event_config_loading branch from a03310f to b00e4fc Compare March 10, 2025 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant