Iox #27 add client and server port to runtime, roudi and process manager [stacked PR #3] #1087

elBoberido · 2022-02-09T23:38:08Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This PR adds the client and server to the Runtime, RouDi and ProcessManager.

The changelog will be updated once the feature is finished.

Test will be added in PR #1137

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
- Each unit test case has a unique UUID
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Relates to Request/Response communication with iceoryx #27

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

elfenpiff · 2022-02-11T09:07:02Z

iceoryx_posh/source/roudi/process_manager.cpp

+                                         const popo::ClientOptions& clientOptions,
+                                         const PortConfigInfo& portConfigInfo) noexcept
+{
+    searchForProcessAndThen(


Side note which makes me wondering. Shouldn't searchForProcessAndThen return an cxx::optional? Then one can write searchForProcess(name).and_then([&](Process & process){ ...

I know it is not the task of this PR but this really bugs me. What do you think, could you make a senior happy by adjusting this?

Done. Happy weekend :)

elfenpiff · 2022-02-11T09:10:29Z

iceoryx_posh/source/roudi/process_manager.cpp

+{
+    searchForProcessAndThen(
+        name,
+        [&](Process& process) { // create a ClientPort


I do not know if we had a rule to avoid generic captures? But what do you think about capturing this, &service, &serverOptions and &name? But I am not insisting only loud thinking.

This goes also for all other lambdas.

Yes, I think we have the rule to explicitly capture. I think the main reason was the ensure we do not capture more than one or two references. For std::function the small object optimization should kick in for less than three references but since we now use cxx::reference_wrapper I'm not sure if this holds anymore. But I think there is an Autosar rule to be explicit with capturing so I will change it anyway.

Since this is legacy code and I already did some cleanup, would you be okay with fixing this when the tools complain and spare some fun for later?

iceoryx_posh/source/roudi/process_manager.cpp

elfenpiff · 2022-02-11T09:17:31Z

iceoryx_posh/source/roudi/roudi.cpp

+    {
+        if (message.getNumberOfElements() != 5)
+        {
+            LogError() << "Wrong number of parameters for \"IpcMessageType::CREATE_CLIENT\" from \"" << runtimeName


Would it be helpful to print the message here for debugging?

There is no NACK send back over the UDS if any of the parameter checks fails, correct? So if a process would send wrong parameters (only possible when runtime does not match) they would be stuck waiting for a response that doesn't come. Maybe they'll get a timeout then?

@elfenpiff I feel this is more a topic for a refactoring for all the messages

@budrus @elfenpiff yes, this is a separate topic. We need to refactor this probably after #1133. I would propose to have something like a CmdHandler class in RouDi which takes care of the actual communication. The ProcessManager would then just return the port to the CmdHandler which in turn sends it to the application. This decouples the ProcessManager from the communication with the client and makes testing easier

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

elfenpiff · 2022-02-11T09:19:55Z

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

+            std::string IpcMessage2 = receiveBuffer.getElementAtIndex(1U);
+            if (stringToIpcMessageType(IpcMessage1.c_str()) == IpcMessageType::ERROR)
+            {
+                LogError() << "Request client received no valid client port from RouDi.";


Would it be helpful to print the received message here?

Maybe not when we serialize it later but at the moment it is human readable ...

(Applies to all occurrences)

Not really. The error is not because of the de-serialization failed but because the response contained the serialized representation of IpcMessageType::ERROR. The second part of the message is the actual error and will be printed later on.

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

codecov · 2022-02-18T17:45:36Z

Codecov Report

Merging #1087 (e414830) into master (ca6fa0f) will decrease coverage by 1.05%.
The diff coverage is 22.67%.

@@            Coverage Diff             @@
##           master    #1087      +/-   ##
==========================================
- Coverage   77.38%   76.33%   -1.06%     
==========================================
  Files         346      346              
  Lines       13389    13580     +191     
  Branches     1917     1935      +18     
==========================================
+ Hits        10361    10366       +5     
- Misses       2396     2584     +188     
+ Partials      632      630       -2

Flag	Coverage Δ
unittests	`75.59% <22.67%> (-1.05%)`	⬇️
unittests_timing	`12.01% <0.00%> (-0.18%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...de/iceoryx_posh/internal/roudi/process_manager.hpp	`100.00% <ø> (ø)`
...posh/include/iceoryx_posh/internal/roudi/roudi.hpp	`80.00% <ø> (ø)`
...eoryx_posh/internal/runtime/ipc_interface_base.hpp	`100.00% <ø> (ø)`
...posh/include/iceoryx_posh/runtime/posh_runtime.hpp	`100.00% <ø> (ø)`
iceoryx_posh/source/roudi/roudi.cpp	`52.72% <0.00%> (-11.01%)`	⬇️
iceoryx_posh/source/runtime/posh_runtime_impl.cpp	`48.61% <0.00%> (-18.06%)`	⬇️
iceoryx_posh/source/roudi/process_manager.cpp	`51.40% <42.65%> (-9.03%)`	⬇️
...e/iceoryx_hoofs/internal/concurrent/smart_lock.inl	`92.45% <0.00%> (-1.89%)`	⬇️
iceoryx_hoofs/source/concurrent/loffli.cpp	`91.42% <0.00%> (+5.71%)`	⬆️

elfenpiff · 2022-02-21T15:58:09Z

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

+                reinterpret_cast<popo::ClientPortUser::MemberType_t*>(ptr));
+        }
+    }
+    else


I think it may not be a technically error but it is somehow unclean.

Assume sendRequestToRouDi fails and returns false but the method changed already parts of the receiveBuffer. So we have somehow a receiveBuffer with crap content and go here into the else branch and check if this crap buffer has 2 elements. This could be successful but would somehow mean undefined behavior.

To fix this possible issue, could you please do an early return

if ( sendRequestToRouDi(sendBuffer, receiveBuffer) ) { // here we require an additional error code we can return when we are unable to send requests to roudi. } ``` and rework the `if` `else` a bit so that you handle explicitly `numberOfElements` `2U` and `3U`.

@elfenpiff done with this commit 2a1280d on #1137

elfenpiff · 2022-02-21T15:59:14Z

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

+PoshRuntimeImpl::requestServerFromRoudi(const IpcMessage& sendBuffer) noexcept
+{
+    IpcMessage receiveBuffer;
+    if (sendRequestToRouDi(sendBuffer, receiveBuffer) && (3U == receiveBuffer.getNumberOfElements()))


We have here the same problem like in requestClientFromRoudi could you do here an early return as well.

elBoberido added the enhancement New feature label Feb 9, 2022

elBoberido self-assigned this Feb 9, 2022

elBoberido linked an issue Feb 9, 2022 that may be closed by this pull request

Request/Response communication with iceoryx #27

Closed

22 tasks

elBoberido changed the title ~~[do-not-review] Iox #27 add client and server port to runtime and roudi [stacked PR #2]~~ [do-not-review] Iox #27 add client and server port to runtime and roudi [stacked PR #3] Feb 9, 2022

elBoberido changed the base branch from master to iox-#27-add-client-and-server-port-to-port-pool-and-port-manager February 9, 2022 23:38

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch 2 times, most recently from 6677cc1 to c1dbf60 Compare February 10, 2022 12:46

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 053fc6e to 8f923cd Compare February 10, 2022 12:49

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from c1dbf60 to 265ef74 Compare February 10, 2022 13:40

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 8f923cd to 47ae0ec Compare February 10, 2022 13:41

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 265ef74 to e35218f Compare February 10, 2022 15:10

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 8132bf2 to 81f8259 Compare February 10, 2022 15:10

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from e35218f to a9ddacd Compare February 10, 2022 15:22

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 81f8259 to 5a5212b Compare February 10, 2022 15:22

elBoberido changed the title ~~[do-not-review] Iox #27 add client and server port to runtime and roudi [stacked PR #3]~~ Iox #27 add client and server port to runtime and roudi [stacked PR #3] Feb 10, 2022

elBoberido changed the title ~~Iox #27 add client and server port to runtime and roudi [stacked PR #3]~~ Iox #27 add client and server port to runtime, roudi and process manager [stacked PR #3] Feb 10, 2022

elfenpiff reviewed Feb 11, 2022

View reviewed changes

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from a9ddacd to 3c9d622 Compare February 11, 2022 11:01

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 5a5212b to e964cf2 Compare February 11, 2022 11:05

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 3c9d622 to 1a53faa Compare February 11, 2022 11:19

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch 2 times, most recently from 41e3fd9 to af415c1 Compare February 11, 2022 18:20

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 9372d15 to d10b138 Compare February 14, 2022 15:20

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from af415c1 to 31d1487 Compare February 14, 2022 15:20

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 34bc993 to 3738316 Compare February 15, 2022 18:44

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch 2 times, most recently from 26e1326 to 4f7cb9c Compare February 15, 2022 19:45

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 3738316 to 2b7e9f4 Compare February 16, 2022 17:49

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 4f7cb9c to 80fecba Compare February 16, 2022 17:52

budrus self-requested a review February 17, 2022 12:05

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch 3 times, most recently from a68ff3e to d2c13c1 Compare February 18, 2022 16:29

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from b6dea2e to cdafb14 Compare February 18, 2022 16:31

elBoberido marked this pull request as ready for review February 18, 2022 18:03

elBoberido requested review from budrus and elfenpiff February 18, 2022 18:05

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from d2c13c1 to 4a550b8 Compare February 18, 2022 18:17

budrus previously approved these changes Feb 20, 2022

View reviewed changes

Base automatically changed from iox-#27-add-client-and-server-port-to-port-pool-and-port-manager to master February 20, 2022 20:33

elBoberido added 8 commits February 21, 2022 12:03

iox-#27 Add client and server port to ProcessManager

0da625d

iox-#27 Add client and server port to RouDi

6810a26

iox-#27 Add client and server port to Runtime

97d4ad3

iox-#27 Refactor ProcessManager::searchForProcessAndThen

ec61bd7

iox-#27 Update copyright

1840124

iox-#27 Update documentation

6a9314a

iox-#27 Improve debug output

b0e21de

iox-#27 Better naming for variable

e414830

elBoberido dismissed budrus’s stale review via e414830 February 21, 2022 11:04

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 13b57d3 to e414830 Compare February 21, 2022 11:04

elBoberido requested a review from budrus February 21, 2022 11:23

elBoberido mentioned this pull request Feb 21, 2022

iox-#27 add client and server port to runtime and roudi tests [stacked PR #3.1] #1137

Merged

21 tasks

budrus approved these changes Feb 21, 2022

View reviewed changes

elfenpiff requested changes Feb 21, 2022

View reviewed changes

elfenpiff approved these changes Feb 21, 2022

View reviewed changes

elBoberido merged commit 8f7fc7f into master Feb 21, 2022

elBoberido deleted the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch February 21, 2022 17:21

elBoberido restored the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch February 21, 2022 20:14

elBoberido deleted the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch February 21, 2022 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #27 add client and server port to runtime, roudi and process manager [stacked PR #3] #1087

Iox #27 add client and server port to runtime, roudi and process manager [stacked PR #3] #1087

elBoberido commented Feb 9, 2022 •

edited

Loading

elfenpiff Feb 11, 2022

elBoberido Feb 11, 2022

elBoberido Feb 11, 2022

elfenpiff Feb 11, 2022

elBoberido Feb 11, 2022

elBoberido Feb 21, 2022

elfenpiff Feb 11, 2022

budrus Feb 17, 2022

budrus Feb 17, 2022

elBoberido Feb 18, 2022

elfenpiff Feb 11, 2022

elBoberido Feb 18, 2022

codecov bot commented Feb 18, 2022 •

edited

Loading

elfenpiff Feb 21, 2022

elBoberido Feb 21, 2022

elfenpiff Feb 21, 2022

Iox #27 add client and server port to runtime, roudi and process manager [stacked PR #3] #1087

Iox #27 add client and server port to runtime, roudi and process manager [stacked PR #3] #1087

Conversation

elBoberido commented Feb 9, 2022 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido commented Feb 9, 2022 •

edited

Loading

codecov bot commented Feb 18, 2022 •

edited

Loading