Skip to content

Fix typed proxy access to generic skeleton event storage#394

Open
rudresh-systream wants to merge 1 commit into
eclipse-score:mainfrom
rudresh-systream:bugfix/311-generic-skeleton-typed-proxy-storage
Open

Fix typed proxy access to generic skeleton event storage#394
rudresh-systream wants to merge 1 commit into
eclipse-score:mainfrom
rudresh-systream:bugfix/311-generic-skeleton-typed-proxy-storage

Conversation

@rudresh-systream
Copy link
Copy Markdown

@rudresh-systream rudresh-systream commented May 8, 2026

Summary:

  • Fixed typed ProxyEvent sample access so it no longer depends on interpreting shared memory as EventDataStorage.
  • Added raw-slot based access using sample size/alignment, making typed proxies compatible with GenericSkeleton-created event storage.
  • Kept existing typed skeleton behavior compatible.
  • Updated ProxyEvent and event storage related tests/resources.

Implemented a compatibility fix for the GenericSkeleton ↔ typed ProxyEvent shared-memory interaction described in #311.

Root cause

The issue originates from the fact that GenericSkeleton and typed skeletons create/interprete EventDataStorage differently.

For typed skeletons, the storage is created through:

EventDataStorage<SampleType>

which internally creates a typed DynamicArray<SampleType>.

For GenericSkeleton/GenericSkeletonEvent, the storage is instead created using:

EventDataStorage<std::max_align_t>

with the storage size being calculated in units of std::max_align_t.

As a result, the underlying raw memory region may be large enough, but the metadata/layout interpretation of the DynamicArray<T> becomes incompatible between the producer and consumer sides.

The typed proxy path was still assuming a typed EventDataStorage<T> representation and therefore interpreted slot count/layout using the wrong type information. This creates an incompatibility whenever:

  • a GenericSkeleton publishes samples
  • and a typed proxy consumes them.

This issue is especially visible because the DynamicArray element count depends on the template type T, which differs between the generic and typed paths.


Investigated solution approaches

Several approaches were considered:

1. Extending DynamicArray<T>

One option was to add a constructor allowing externally managed/preallocated storage while preserving a typed DynamicArray<T> interface.

This was not chosen because:

  • it would introduce LoLa/event-storage specific semantics into a generic container abstraction,
  • it increases complexity in DynamicArray,
  • and it still keeps the shared-memory interpretation tightly coupled to template typing.

2. Replacing all event storage with DynamicArray<std::byte>

Another option was to fully migrate all event storage handling to raw byte storage.

While architecturally clean, this would require significantly broader refactoring across:

  • typed skeletons,
  • generic skeletons,
  • proxies,
  • allocation logic,
  • and existing tests.

Given the scope/risk, this approach was considered too invasive for the current issue.

3. Fixing typed proxy access to use raw slot storage (chosen solution)

The implemented solution changes typed proxy sample access so it no longer depends on interpreting the shared memory as EventDataStorage<T>.

Instead:

  • the proxy accesses the shared-memory event region through raw slot memory/meta information,

  • and calculates slot/sample access using:

    • sizeof(T)
    • alignof(T)

This keeps the existing shared-memory layout compatible while allowing typed proxies to correctly consume data produced by GenericSkeleton.

This approach was selected because it:

  • directly resolves the interoperability problem,
  • minimizes architectural disruption,
  • preserves existing typed skeleton behavior,
  • avoids changing generic container semantics,
  • and provides a more robust producer/consumer compatibility model going forward.

Files changed

score/mw/com/impl/bindings/lola/proxy_event.h

Main functional fix.

Updated typed ProxyEvent<T> sample access logic to avoid relying on interpreting the shared-memory region as EventDataStorage<T>.

Instead, access is now performed through raw event slot metadata and pointer arithmetic based on the actual sample type size/alignment.

This makes typed proxies compatible with GenericSkeleton-created storage.


score/mw/com/impl/bindings/lola/skeleton_memory_manager.h

score/mw/com/impl/bindings/lola/skeleton_memory_manager.cpp

Updated/supporting changes around event storage creation and raw slot metadata handling.

These changes ensure both generic and typed paths expose compatible storage information to consumers.


score/mw/com/impl/bindings/lola/skeleton.cpp

Adjusted skeleton-side integration to align with the updated event storage access model and metadata usage.


score/mw/com/impl/bindings/lola/proxy_event_test.cpp

score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.cpp

score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.h

Updated and extended test resources/coverage to validate:

  • typed proxy interaction with GenericSkeleton-created event storage,
  • slot access correctness,
  • and compatibility of the updated shared-memory interpretation model.

Validation performed

The following builds/tests were executed successfully after the changes:

bazel build //score/mw/com/impl/bindings/lola:proxy
bazel build //score/mw/com/impl/bindings/lola:skeleton

bazel test //score/mw/com/impl/bindings/lola:proxy_event_test
bazel test //score/mw/com/impl/bindings/lola:generic_proxy_event_test
bazel test //score/mw/com/impl/bindings/lola:skeleton_test
bazel test //score/mw/com/impl/bindings/lola:event_data_storage_test

All relevant LoLa event/proxy/skeleton tests passed successfully after the fix.

@rudresh-systream rudresh-systream force-pushed the bugfix/311-generic-skeleton-typed-proxy-storage branch from 37ebeb3 to dcb34ed Compare May 8, 2026 11:01
@crimson11
Copy link
Copy Markdown
Contributor

@rudresh-systream - thank you for your effort/fix proposal. @bemerybmw and me hat a quick 1st look. Looks promising as a short-term bugfix-solution! Will do a deeper review soon.

@ShoroukRamzy
Copy link
Copy Markdown
Contributor

Hi @rudresh-systream and @crimson11, I quickly reviewed this fix and also run our integration test #379 and all test cases passed for generic-typed/generic use cases.
Thanks for the PR!

Comment thread score/mw/com/impl/bindings/lola/skeleton_memory_manager.h Outdated
Comment thread score/mw/com/impl/bindings/lola/proxy_event.h Outdated
Comment thread score/mw/com/impl/bindings/lola/proxy_event.h Outdated
Comment thread score/mw/com/impl/bindings/lola/skeleton_memory_manager.h Outdated
@rudresh-systream rudresh-systream force-pushed the bugfix/311-generic-skeleton-typed-proxy-storage branch from f015563 to fcbca26 Compare May 21, 2026 09:58
@rudresh-systream rudresh-systream force-pushed the bugfix/311-generic-skeleton-typed-proxy-storage branch from c93dee0 to 29563be Compare May 22, 2026 08:59
@rudresh-systream
Copy link
Copy Markdown
Author

@crimson11
I removed the duplicate GenericSkeleton typed-proxy integration test/application from this PR and force-pushed the branch as a single squashed commit.

This PR is now focused on the #311 production fix and the remaining focused unit/regression coverage. I will check PR #379 for the GenericSkeleton integration-test coverage and add review feedback there if I find any missing Generic-Typed coverage relevant to this issue.

Comment thread score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.h
Comment thread score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.h Outdated
Comment thread score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.cpp Outdated
Comment thread score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.cpp Outdated
@rudresh-systream rudresh-systream force-pushed the bugfix/311-generic-skeleton-typed-proxy-storage branch from a94f24f to 642fdd7 Compare May 22, 2026 11:22
Comment thread score/mw/com/impl/bindings/lola/proxy_event_test.cpp Outdated
Comment thread score/mw/com/impl/bindings/lola/proxy_event_test.cpp
Comment thread score/mw/com/impl/bindings/lola/proxy_event_test.cpp Outdated
Comment thread score/mw/com/impl/bindings/lola/proxy_event_test.cpp
Comment thread score/mw/com/impl/bindings/lola/skeleton_memory_manager.cpp Outdated
Comment thread score/mw/com/impl/bindings/lola/proxy_event.h Outdated
Comment thread score/mw/com/impl/bindings/lola/proxy_event.h Outdated
Comment thread score/mw/com/impl/bindings/lola/proxy_event.h Outdated
Comment thread score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.h
Comment thread score/mw/com/impl/bindings/lola/test_doubles/fake_mocked_service_data.h Outdated
Comment thread score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.cpp
Typed ProxyEvent now accesses event samples through EventMetaInfo raw slot storage instead of interpreting provider storage as EventDataStorage<SampleType>.

This keeps typed proxies compatible with GenericSkeleton-created storage while preserving typed skeleton behavior. Proxy event tests and fixtures were updated to emulate production typed storage with a type-erased EventMetaInfo view.
@rudresh-systream rudresh-systream force-pushed the bugfix/311-generic-skeleton-typed-proxy-storage branch from 1f6c681 to 95502db Compare May 25, 2026 06:23
@rudresh-systream
Copy link
Copy Markdown
Author

@bemerybmw

Summary of changes:

  • Reworked the added regression test to use a behavior-based name and Given / When / Then structure.
  • Removed unnecessary RecordProperty metadata from the added regression test.
  • Kept SampleConstness focused on sample constness and added a separate EventMetaInfo const-reference test.
  • Changed raw-slot size calculations to use safe_math::ReturnMode::kAbortOnError.
  • Changed InitialiseEventSlotsRawArray() to return const std::uint8_t* and initialize event_slots_raw_array_ directly.
  • Removed noexcept from InitialiseEventSlotsRawArray().
  • Added the requested TODO explaining that raw-slot access is a temporary compatibility fix until the LoLa binding layer is type-erased.
  • Restored FakeMockedServiceData::AddEvent() to return EventDataStorage*.
  • Restored event_data_storage_ in the proxy-event fixture and made event_slots_raw_array_ a type-erased view into event_data_storage_->data().
  • Rebased onto latest origin/main, squashed to one commit, removed merge commits, and removed the Signed-off-by trailer.

Validation:

  • bazel test //:format_test_C++_with_clang-format --test_output=all
  • bazel build //score/mw/com/impl/bindings/lola:proxy
  • bazel build //score/mw/com/impl/bindings/lola:skeleton
  • bazel test //score/mw/com/impl/bindings/lola:proxy_event_test
  • bazel test //score/mw/com/impl/bindings/lola:generic_proxy_event_test
  • bazel test //score/mw/com/impl/bindings/lola:skeleton_test
  • bazel test //score/mw/com/impl/bindings/lola:event_data_storage_test

Final branch state:

  • one commit on top of origin/main
  • no merge commits
  • no Signed-off-by trailer

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.

4 participants