iox-#590 Remove error handler calls from hoofs #1027

mossmaurice · 2022-01-19T14:40:25Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

Templatize ErrorHandler::setTemporaryErrorHandler to prepare usage in different modules (see iox-#590 Separate errors of posh and C binding #1028 )
Call errorHandler() in hoofs only from cxx::Require() as described in the error handling document

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

Closes Improve modularity by creating architecure guidelines #590

mossmaurice · 2022-01-19T15:24:23Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

@@ -17,20 +17,23 @@
 #ifndef IOX_HOOFS_ERROR_HANDLING_ERROR_HANDLING_HPP
 #define IOX_HOOFS_ERROR_HANDLING_ERROR_HANDLING_HPP

+/// @todo #590 rename this file to error_handler.hpp


Done in #1028

This is hard. Furthermore #1028 states the following: Separate errors of posh and C binding and as it seems currently the include guards, include headers, cmake files are not yet adjusted there. Please create a separate PR for this the other one is already huge.

On the 2nd thought, I think what we need to do is separate the ErrorHandler from error_handling.hpp. iceoryx_hoofs's error_handling.hpp will have only one entry EXPECTS_ENSURES_FAILED. So in the end:

error_handler.hpp (hoofs)

error_handling.hpp (hoofs)

error_handling.hpp (posh)

error_handling.hpp (C binding)

Added to #1099

mossmaurice · 2022-01-19T15:24:35Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

-                  const std::function<void()>& errorCallBack = std::function<void()>(),
-                  const ErrorLevel level = ErrorLevel::FATAL) noexcept;
+/// @tparam[in] Error type of the enum which is used to report the error
+/// @todo use enable_if with is_enum


Done in #1028

mossmaurice · 2022-01-19T15:24:43Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

+    ErrorHandler<Error>::handler(error, errorCallBack, level);
+}
+
+/// @todo #590 specialise for posh and c binding error handling


Done in #1028

mossmaurice · 2022-01-19T15:24:54Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

+    }
+}
+
+/// @todo #590 move the below to .inl


Done in #1028

codecov · 2022-01-19T20:47:33Z

Codecov Report

Merging #1027 (69d9448) into master (2900bd4) will decrease coverage by 0.02%.
The diff coverage is 73.07%.

❗ Current head 69d9448 differs from pull request most recent head fd0b941. Consider uploading reports for the commit fd0b941 to get more accurate results

@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
- Coverage   76.17%   76.15%   -0.03%     
==========================================
  Files         343      345       +2     
  Lines       13003    13008       +5     
  Branches     1870     1861       -9     
==========================================
+ Hits         9905     9906       +1     
- Misses       2481     2492      +11     
+ Partials      617      610       -7

Flag	Coverage Δ
unittests	`75.39% <61.53%> (-0.02%)`	⬇️
unittests_timing	`12.46% <15.38%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
...clude/iceoryx_hoofs/internal/cxx/variant_queue.inl	`78.31% <ø> (ø)`
...iceoryx_hoofs/internal/file_reader/file_reader.hpp	`100.00% <ø> (ø)`
...eoryx_posh/internal/runtime/ipc_interface_base.hpp	`100.00% <ø> (ø)`
...oryx_posh/include/iceoryx_posh/roudi/roudi_app.hpp	`40.00% <ø> (ø)`
iceoryx_hoofs/source/file_reader/file_reader.cpp	`71.42% <50.00%> (-6.84%)`	⬇️
iceoryx_hoofs/source/posix_wrapper/timer.cpp	`64.68% <80.00%> (+1.61%)`	⬆️
...de/iceoryx_hoofs/error_handling/error_handling.hpp	`100.00% <100.00%> (ø)`
...ryx_hoofs/source/error_handling/error_handling.cpp	`60.00% <100.00%> (-7.57%)`	⬇️
...six_wrapper/shared_memory_object/shared_memory.cpp	`64.73% <0.00%> (-4.30%)`	⬇️
... and 5 more

elfenpiff

Can we please keep this PR on hold until we figured out how the overall error concept in iceoryx looks like.
To call terminate is most likely not an option for ASIL D and we may have to make this either configurable or that some user can define custom callbacks to notify maybe some kind of watchdog or even throw an exception.

In those cases an error handler may is the more viable solution.

MatthiasKillat · 2022-01-20T13:31:29Z

From a quick view I think the approach is fine and the scope well-defined. It is important that the error handling semantics will not be changed for now, only the syntax and organization of the code.

I.e. we check for the same conditions as before and react in the same way, just use different code. In other words there is no regression and also no new handling introduced.

This is a first step to allow individual error-handling for different modules. Changes of the overall concept should indeed be postponed and require more discussion. We will not have any overall concept change in iceoryx 2.0.

NB: this is just a first impression, I will have a closer look tomorrow.

mossmaurice · 2022-01-20T15:56:12Z

@elfenpiff

Can we please keep this PR on hold until we figured out how the overall error concept in iceoryx looks like. To call terminate is most likely not an option for ASIL D and we may have to make this either configurable or that some user can define custom callbacks to notify maybe some kind of watchdog or even throw an exception.

In those cases an error handler may is the more viable solution.

Thanks for the remark, got your point. As we're not 100% sure how the final concept will look like, I would suggest to take an iterative approach here. My proposal is to get step by step to the full blown concept. Two goals are reached in #1027 and #1028:

Cleaner architecture: hoofs is independent from both posh and C binding
We are able to detect based on the enum type which module reported the error

As @MatthiasKillat pointed out, there is no semantic change. A next step towards the final design could be adding hooks to report and persistently save all errors from any module. This would enable reading them e.g. via UDS (both in Expects/Ensures and errorHandler).

elBoberido · 2022-01-21T10:44:56Z

@elfenpiff @MatthiasKillat @mossmaurice
I also don't think this should be kept on hold. Like already said, it is a small iterative step and does not change the concept by much. I wouldn't call the cxx::Expects in the error handler though but the other way around. The cxx::Expects should call the errror handler. At one point we have to delegate the error code to a 3rd party and cxx::Expects cannot do this since the error value is already erased. So after some thinking about this, I would keep the calls to the error handler in hoofs but create separate enums for each component. The cxx::Expects should also have an entry in this enum, e.g. EXPECTS_CONDITION_FAILED.

I felt asleep on the keyboard and when I woke up I had a PoC of a refactored logger on my laptop. @elfenpiff told me that it also happens to him at times. Might be a virus and seems contagious.
I think the same concept could be used to make the error handler more ergonomic and provide a hook for a 3rd party framework. I'll create a design document draft for the logger refactoring and then we can discuss if this would also be an option for the error handler. Small teaser, I envision something like this

iox_error(FATAL, HoofsError::POSIX_SEMAPHORE_FOO) << "what the heck: " << 42 \
    << "! This is not supposed to happen";

Btw, I'm wondering for what the custom error callback is good for? Do we need it? Can we remove it?
I had a quick grep over the codebase and we never provide a callback but I had some fun on #861 because it was accidentally used. We should kill it with fire but not in this PR.

elBoberido

I don't see a reason to create the template. When this is specialized we would essentially need to duplicate the code multiple times. It is much easier to only have a template function which erases the enum class and provides the error string to the error handler, e.g.

template <typename E>
void errorHandlerWrapper(E error, callback, ErrorLevel) {
   errorHandler(static_cast<underlying_type>(error), to_string(error), callback, ErrorLevel);
}

Did I misunderstood something? If not, please revert those changes

elBoberido · 2022-01-21T10:52:04Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

+///         - defaultHandler(const NewEnumErrorType error, const std::function<void()>& errorCallBack, const
+///         ErrorLevel level)
+///         - std::ostream& operator<<(std::ostream& stream, NewEnumErrorType value)
+///         - const char* toString(const NewEnumErrorType error)


I'm not quite sure why this is necessary, at least not for defaultHandler and operator<<. Maybe something further down the PR will brighten this up.

elBoberido · 2022-01-21T11:00:40Z

iceoryx_hoofs/test/moduletests/test_posix_timer.cpp

-    std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT.toMilliseconds() * 10));
-
-    TIMING_TEST_EXPECT_TRUE(hasTerminated);
+    EXPECT_DEATH(


AFAIK the timer already started a thread and gtest will print warnings when a process with running threads is forked. We play Russian roulette with this and on a previous PR I had to disable one of these tests because it hang on macOS. Maybe it is not the worst idea to use the error handler in this case and get rid of most of the death tests.

Slightly offtopic but in this case we might also want to delete this class since it is not used in iceoryx and will also not be certified in this state.

Yes, as far as I know fork has no information about any child threads and the resources (e.g. mutex) they use. I think it will copy the entire process space but only have the clone of the calling thread running in the fork Everything else may not run or is undefined, the threads may not have their own proper mutexes etc. (though their states are somewhat copied in memory, see below).

Likely this is called on linux.
https://man7.org/linux/man-pages/man2/fork.2.html

TLDR: I am not sure whether the threads in a forked process can operate properly and have the resources they need (independent from the original). From the above I think this is not the case and a tricky problem. As DEATH tests fork, we cannot use them if multiple threads run (or need to understand the limitations, I do not fully understand them).

@elBoberido Agree 100%, DEATH_TESTS should therefore always be avoided. Let's remove the posix::Timer together with resolving #754.

MatthiasKillat

Fine by me and a step in a direction to modular/custom error handling that I like. I mentioned some proposals and potential improvements for the future. Requires some more thought and iterations to really arrive at something generic and powerful but there is no immediate pressure to do so.

MatthiasKillat · 2022-01-21T12:26:02Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

@@ -209,20 +205,22 @@ enum class ErrorLevel : uint32_t
    MODERATE
 };

+template <typename Error>
 using HandlerFunction = std::function<void(const Error error, const std::function<void()>, const ErrorLevel)>;


Maybe at some point we can try out cxx::function. The main downside is that we have to limit the size of the callable to something reasonable (mainly of interest for heavyweight functors).

Not in this PR though, I will replace some internal std::function before we do this. For a dynamic memory free version (which I think we want to have in the long run) we need to avoid std::function.

Agree. If time is running out for v2.0 then definitely shortly after

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

MatthiasKillat · 2022-01-21T13:00:54Z

iceoryx_hoofs/source/file_reader/file_reader.cpp

-            }
-            }
-        });
+        switch (m_errorMode)


Is semantically (almost) equivalent (the code Error::kFILEREADER__FAILED_TO_OPEN_FILE was removed, intentionally I presume to have no such codes in hoofs).

It looks a bit weird though in general, as is kind of invokes its own error handling inside the switch. If everything is generalized properly, the error handler should handle the decision whether to terminate etc. based on a severity level and a safety mode (ErrorMode is kind of like this).

The user only should provide the safety/handling mode, error severity and optional custom code/messages. It is important to have the code as concise as possible for this but also used in the same way in all places.

NB: I am just trying to think through how it could ideally work in the future, nothing to change here for now.

MatthiasKillat · 2022-01-21T13:05:36Z

iceoryx_hoofs/source/posix_wrapper/timer.cpp

-                errorHandler(Error::kPOSIX_TIMER__INCONSISTENT_STATE);
-                return;
-            }
+            cxx::Expects((handle.m_timer != nullptr) && "Timer in inconsistent state");


In a future version of Expects the message should be a separate (optional?) argument and not appended with && like in the good old days for assert in C.

Only a style issue and personal opinion though, nothing for this PR unless this is already possible.

MatthiasKillat · 2022-01-21T13:08:39Z

iceoryx_hoofs/source/posix_wrapper/timer.cpp

-            errorHandler(Error::kPOSIX_TIMER__CALLBACK_RUNTIME_EXCEEDS_RETRIGGER_TIME);
-        }
-        return;
+        cxx::Ensures((handle.m_catchUpPolicy != CatchUpPolicy::TERMINATE)


Personally I think the distinction between Ensuresand Expects is arbitrary and not needed. Especially if I can and want to call either in the middle of a function. The names are also not meaningfully distinct.

We just need one of them and might as well call it Assert (though I am fine with Expect as well). Just a better flow when reading the code IMO (also closer to say GTest, where you also expect some condition to hold at specific points of the test).

I put Requires to the polls 😆

MatthiasKillat · 2022-01-21T13:13:04Z

iceoryx_hoofs/test/moduletests/test_posix_timer.cpp

    std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT.toMilliseconds() * 10));
-    TIMING_TEST_EXPECT_FALSE(hasTerminated);
+    // EXPECT_NO_DEATH
+    EXPECT_TRUE(true);


This makes no sense from my point of view (tautology). If we reach the code, the test passes anyway (and is listed as successful). Otherwise we have crashed and the line does nothing.

I know that it is repeated at most 5 times and needs to pass at least once, but is this really meaningful/needed?

I know what you mean. However, this is ok for now, as the posix::Timer will be removed before the iceoryx v2.0 anyway.

I'm fine with the comment, but for future cases GTEST_SUCCEED() << "Expecting no death"; might be a good alternative.

MatthiasKillat · 2022-01-21T13:16:02Z

iceoryx_hoofs/test/moduletests/test_posix_timer.cpp

    std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT.toMilliseconds() * 10));
-    TIMING_TEST_EXPECT_FALSE(hasTerminated);
+    // EXPECT_NO_DEATH
+    EXPECT_TRUE(true);


Required and meaningful? See above.

MatthiasKillat · 2022-01-21T13:17:41Z

iceoryx_hoofs/test/moduletests/test_posix_timer.cpp

    std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT.toMilliseconds() * 10));
-    TIMING_TEST_EXPECT_FALSE(hasTerminated);
+    // EXPECT_NO_DEATH
+    EXPECT_TRUE(true);
 });

 TIMING_TEST_F(Timer_test, CatchUpPolicyTerminateTerminatesWhenCallbackIsLongerThenTriggerTime, Repeat(5), [&] {


More like ...CallbackRunsLonger... but functionally irrelevant.

MatthiasKillat · 2022-01-21T13:35:49Z

iceoryx_hoofs/test/moduletests/test_posix_timer.cpp

-    std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT.toMilliseconds() * 10));
-
-    TIMING_TEST_EXPECT_TRUE(hasTerminated);
+    EXPECT_DEATH(


Yes, as far as I know fork has no information about any child threads and the resources (e.g. mutex) they use. I think it will copy the entire process space but only have the clone of the calling thread running in the fork Everything else may not run or is undefined, the threads may not have their own proper mutexes etc. (though their states are somewhat copied in memory, see below).

Likely this is called on linux.
https://man7.org/linux/man-pages/man2/fork.2.html

TLDR: I am not sure whether the threads in a forked process can operate properly and have the resources they need (independent from the original). From the above I think this is not the case and a tricky problem. As DEATH tests fork, we cannot use them if multiple threads run (or need to understand the limitations, I do not fully understand them).

MatthiasKillat · 2022-01-21T13:43:45Z

iceoryx_posh/test/integrationtests/test_posh_mepoo.cpp

@@ -416,7 +416,7 @@ TEST_F(Mepoo_IntegrationTest, WrongSampleSize)
    constexpr uint32_t SAMPLE_SIZE = 2048U;
    constexpr uint32_t REPETITION = 1U;
    iox::cxx::optional<iox::Error> receivedError;
-    auto errorHandlerGuard = iox::ErrorHandler::setTemporaryErrorHandler(
+    auto errorHandlerGuard = iox::ErrorHandler<iox::Error>::setTemporaryErrorHandler(


We may be able to get rid of the explicit template argument by defining a specific error handler alias (via using) in the ''posh' namespace. This could actually be good practice and leads to modular error handling without. We would then call the iox::posh::ErrorHandler explicitly or implicitly (the latter may be undesirable if the error handler is not configured correctly for all namespaces where it is called).

@elBoberido

We should consider avoiding the class template argument, as @elBoberido stated.

MatthiasKillat · 2022-01-21T14:12:23Z

I retracted my approval because I am not sure whether we can get rid of the template argument in

template <typename Error>
class ErrorHandler

I would like to if possible, as would @elBoberido if I understand it correctly. I just to not fully understand all implications and goals.

The template argument has the advantage the we actually enforce what kind or error this particular handler can handle at compile time (in contrast to providing a template method where the type is variable). This is only partially possible with template methods (would need enable_if which gets out of hand quickly)

This is closely related to the usage like auto errorHandlerGuard = iox::ErrorHandler<iox::Error>::setTemporaryErrorHandler ... .
I would like to somehow define it in a way that enforces to automatically call the correct error handler in the whole posh namespace etc. with the right error handler, maybe with a using directive in the module namespace. This alias will need to be called explicitly though

Sorry, this is not fully thought out on my side, I need to think this through over the weekend.
It is mainly a usage problem and I am not fully aware whether the goal is to actually enforce that some handler can only handle specific error types. Functionally it is all fine.

I think if we cannot find a (simple) adjustment we should merge it and refactor later. It is still an improvement in my book.

elfenpiff

You checked the PR title check but the title does not follow our guidelines. Please add iox-#???.

mossmaurice · 2022-01-24T16:59:23Z

@elBoberido Thanks for the review.

The cxx::Expects should call the errror handler. At one point we have to delegate the error code to a 3rd party and cxx::Expects cannot do this since the error value is already erased.

I think you mixed something up. Probably you meant the FileReader? This is a bit misleading to see in the code, but this is not the error mode of the ErrorHandler but of the FileReader itself. Hence, I completely agree with The cxx::Expects should call the errror handler. ✔️

The cxx::Expects should also have an entry in this enum, e.g. EXPECTS_CONDITION_FAILED.

Will be implemented by #1035

Btw, I'm wondering for what the custom error callback is good for? Do we need it? Can we remove it?

That's true indeed 👍

I don't see a reason to create the template.

Well, setTemporaryErrorHandler is the reason I templatized, but you're right it's possible to erase the type and just use the underlying integer value. I'll give it a try.

…er() calls from hoofs Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice · 2022-02-07T17:22:05Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

+/// @todo #590 move the implementations below to .inl
+template <typename Error>
+inline void errorHandler(const Error error,
+                         const std::function<void()>& errorCallBack IOX_MAYBE_UNUSED,


The removal of the 2nd arg will be done either in #1028 or a follow-up.

Maybe we should create small separate issues for the error handler since I am already a little bit lost. And you have several comments where you state, will be done in this PR or will be done in another follow up or in this issue and so on. And when I look at those PRs, issues etc. either the task is done incompletely or nothing is mentioned in the issue to this particular task.

For me as a reviewer it is hard to track all of this.

I'll close this PR and will open an new one based on #1099

…oid problems in tests Signed-off-by: Simon Hoinkis <[email protected]>

elBoberido · 2022-02-07T17:49:45Z

I'll ask the heretical question, is this relevant for v2? The code freeze is not that far away and it binds resources on the reviewer side and on developer fixing merge conflicts due to breaking API changes. If we have too many of this side topics I fear we might run out of time at the end of February :/
This is not only related to your PR but in general to all PRs we currently have.

Signed-off-by: Simon Hoinkis <[email protected]>

…sh_testing Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

iceoryx_binding_c/test/moduletests/test_c2cpp_enum_translation.cpp

mossmaurice · 2022-02-08T13:06:25Z

I'll ask the heretical question, is this relevant for v2? The code freeze is not that far away and it binds resources on the reviewer side and on developer fixing merge conflicts due to breaking API changes. If we have too many of this side topics I fear we might run out of time at the end of February :/ This is not only related to your PR but in general to all PRs we currently have.

I understand what you mean. First prio are definitely the features #415 and #27. However, as this PR solves the last "illegal" dependency from iceoryx_hoofs to iceoryx_posh, I'd appreciate it a lot if we can merge it before v2.0. A clean architecture will enable us faster development in future. I have several other things in mind for v2.0 that are on the board.

Signed-off-by: Simon Hoinkis <[email protected]>

elBoberido

Looks good. Just minor things. Could also be merged as is.

elBoberido · 2022-02-09T00:32:05Z

iceoryx_binding_c/test/moduletests/test_c2cpp_enum_translation.cpp

@@ -58,8 +58,8 @@ TEST(c2cpp_enum_translation_test, SubscriberState)
 // the clang sanitizer detects this successfully and this leads to termination, and with this the test fails
 #if !defined(__clang__)
    iox::Error errorValue = iox::Error::kNO_ERROR;
-    auto errorHandlerGuard = iox::ErrorHandler::setTemporaryErrorHandler(
-        [&](const iox::Error e, const std::function<void()>, const iox::ErrorLevel) { errorValue = e; });
+    auto errorHandlerGuard = iox::ErrorHandlerMock::setTemporaryErrorHandler<iox::Error>(


Even though I suggested ErrorHandlerMock and I introduce a LoggerMock, I'm not fully convinced by this name. This is not a request to change it, because currently I can't suggest a better name but if you or another reviewer can find one I would highly appreciate it ... and while writing this, TestingErrorHandler/testing::ErrorHandler came to my mind. What do you think, is this better or worse?

I was thinking about this also back and forth. My conclusion was: The ErrorHandlerMock is not really a mock more of a ErrorHandlerHook. I kept the name as the term Mock will trigger a "Hey this is only for tests"-thought when developer read it. That's why I think we should both keep LoggerMock and ErrorHandlerMock. Other names could be misleading.

Yeah, I'm a bit torn on this but like I said, I don't have an idea where I would say it is much better than the current naming.

If it has full functionality and is an error handler used in tests it should not be called a Mock of any kind. I think TestingErrorHandler or TestErrorHandler is better for this reason.

elBoberido · 2022-02-09T00:37:04Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

+///     2.) Specialize the following methods for your NewEnumErrorType:
+///         - const char* toString(const NewEnumErrorType error)
+///
+///     3.) Call errorHandler(Error::kMODULE_NAME__MY_FUNKY_ERROR);


I can't recall the decision on the k prefix. Did we decide to remove it? If yes, I guess it will be done in a follow up PR together with the removal of errorCallback?

There wasn't a decision on that topic yet. If we will change it, I would for sure do it in a follow-up PR.

Okay, please keep in mind to let us come to a decision before the callback is removed from the errorHandler since it would fit quite nice with such a PR

@elBoberido @mossmaurice I think we introduced the k prefix so that it conforms to the autosar guidelines and then we found out that this is actually pretty nice since otherwise we would run into some trouble with our macro.

So whoever does this - be prepared for fun :D

@elfenpiff that's no problem. One has just to replace #define CREATE_ICEORYX_ERROR_ENUM(name) k##name, with #define CREATE_ICEORYX_ERROR_ENUM(name) name,

elBoberido · 2022-02-09T00:40:40Z

iceoryx_hoofs/source/error_handling/error_handling.cpp

 namespace iox
 {
-const char* ErrorHandler::ERROR_NAMES[] = {ICEORYX_ERRORS(CREATE_ICEORYX_ERROR_STRING)};
+const char* ERROR_NAMES[] = {ICEORYX_ERRORS(CREATE_ICEORYX_ERROR_STRING)};


Not sure if this is a good idea since it might clash with ERROR_NAMES from posh once the posh errors are moved there. What was the reason to move it out of the class scope?

Have a look at #1028. The names will be unique for each module e.g. C_BINDING_ERROR_NAMES[]. Hence no clashes are possible.

Okay, so it will be taken care of when the split actually happens

I think we have to do some restructuring. Issue #590 states nothing about the error handling, we have several hints that certain tasks are performed in several PRs or in this issue but this is nowhere really tracked or mentioned.

Why is the error handler part of #590 ? (I know why but this is hard to keep track when this is nowhere mentioned)

That's true. I'll close this PR here and re-open it as part of a new issue.

elBoberido · 2022-02-09T00:49:33Z

iceoryx_hoofs/test/moduletests/test_posix_timer.cpp

    std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT.toMilliseconds() * 10));
-    TIMING_TEST_EXPECT_FALSE(hasTerminated);
+    // EXPECT_NO_DEATH
+    EXPECT_TRUE(true);


I'm fine with the comment, but for future cases GTEST_SUCCEED() << "Expecting no death"; might be a good alternative.

elBoberido · 2022-02-09T00:54:40Z

iceoryx_hoofs/testing/include/iceoryx_hoofs/testing/mocks/error_handler_mock.hpp

+    uint32_t errorEnumType = error >> 16;
+    uint32_t expectedErrorEnumType =
+        static_cast<typename std::underlying_type<ErrorEnumType>::type>(ErrorEnumType::kNO_ERROR) >> 16;


I'm surprised. You convinced me of the struct but are now using shifting 😆
I guess it is just to do not break to much at a time :)

iceoryx_hoofs/testing/include/iceoryx_hoofs/testing/mocks/error_handler_mock.hpp

elfenpiff

Please create another issue where you only explain the work which is done here on the error handler. Issue #590 should have a todo list which links to all the separate issues. Then we have nice small understandable issues where we can keep track of stuff.

iceoryx_binding_c/test/test.hpp

iceoryx_binding_c/test/moduletests/test_c2cpp_enum_translation.cpp

elfenpiff · 2022-02-09T12:09:58Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

@@ -17,20 +17,23 @@
 #ifndef IOX_HOOFS_ERROR_HANDLING_ERROR_HANDLING_HPP
 #define IOX_HOOFS_ERROR_HANDLING_ERROR_HANDLING_HPP

+/// @todo #590 rename this file to error_handler.hpp


This is hard. Furthermore #1028 states the following: Separate errors of posh and C binding and as it seems currently the include guards, include headers, cmake files are not yet adjusted there. Please create a separate PR for this the other one is already huge.

elfenpiff · 2022-02-09T12:13:03Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

+///     2.) Specialize the following methods for your NewEnumErrorType:
+///         - const char* toString(const NewEnumErrorType error)
+///
+///     3.) Call errorHandler(Error::kMODULE_NAME__MY_FUNKY_ERROR);


@elBoberido @mossmaurice I think we introduced the k prefix so that it conforms to the autosar guidelines and then we found out that this is actually pretty nice since otherwise we would run into some trouble with our macro.

So whoever does this - be prepared for fun :D

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

elfenpiff · 2022-02-09T12:21:45Z

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

+/// @todo #590 move the implementations below to .inl
+template <typename Error>
+inline void errorHandler(const Error error,
+                         const std::function<void()>& errorCallBack IOX_MAYBE_UNUSED,


Maybe we should create small separate issues for the error handler since I am already a little bit lost. And you have several comments where you state, will be done in this PR or will be done in another follow up or in this issue and so on. And when I look at those PRs, issues etc. either the task is done incompletely or nothing is mentioned in the issue to this particular task.

For me as a reviewer it is hard to track all of this.

elfenpiff · 2022-02-09T12:24:24Z

iceoryx_hoofs/source/error_handling/error_handling.cpp

 namespace iox
 {
-const char* ErrorHandler::ERROR_NAMES[] = {ICEORYX_ERRORS(CREATE_ICEORYX_ERROR_STRING)};
+const char* ERROR_NAMES[] = {ICEORYX_ERRORS(CREATE_ICEORYX_ERROR_STRING)};


I think we have to do some restructuring. Issue #590 states nothing about the error handling, we have several hints that certain tasks are performed in several PRs or in this issue but this is nowhere really tracked or mentioned.

Why is the error handler part of #590 ? (I know why but this is hard to keep track when this is nowhere mentioned)

iceoryx_hoofs/testing/error_handler_mock.cpp

MatthiasKillat · 2022-02-09T13:47:31Z

iceoryx_binding_c/test/moduletests/test_c2cpp_enum_translation.cpp

@@ -95,8 +95,8 @@ TEST(c2cpp_enum_translation_test, SubscriberEvent)
 // the clang sanitizer detects this successfully and this leads to termination, and with this the test fails
 #if !defined(__clang__)
    iox::Error errorValue = iox::Error::kNO_ERROR;
-    auto errorHandlerGuard = iox::ErrorHandler::setTemporaryErrorHandler(
-        [&](const iox::Error e, const std::function<void()>, const iox::ErrorLevel) { errorValue = e; });
+    auto errorHandlerGuard = iox::ErrorHandlerMock::setTemporaryErrorHandler<iox::Error>(


Except that std::function is gone this does not look and feel better (naming with Mock, template arg) to use. We can fix this later of course but really need to think about how this could be used in a nice way.

…unexpected error enum type in error handler Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

…todo Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice · 2022-02-12T22:35:31Z

Superseded by #1100, hence closing this PR here.

mossmaurice added the refactoring Refactor code without adding features label Jan 19, 2022

mossmaurice requested review from elfenpiff and elBoberido January 19, 2022 14:40

mossmaurice self-assigned this Jan 19, 2022

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch from 6a3ccf2 to 4b49eed Compare January 19, 2022 14:44

mossmaurice requested a review from MatthiasKillat January 19, 2022 14:45

mossmaurice mentioned this pull request Jan 19, 2022

iox-#590 Separate errors of posh and C binding #1028

Closed

21 tasks

mossmaurice commented Jan 19, 2022

View reviewed changes

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch from bfdf9b7 to 11b3c97 Compare January 19, 2022 20:31

elfenpiff reviewed Jan 20, 2022

View reviewed changes

elfenpiff approved these changes Jan 21, 2022

View reviewed changes

elBoberido reviewed Jan 21, 2022

View reviewed changes

elfenpiff mentioned this pull request Jan 21, 2022

ErrorHandling concept with user defined actions #1032

Open

2 tasks

MatthiasKillat previously approved these changes Jan 21, 2022

View reviewed changes

elfenpiff requested changes Jan 24, 2022

View reviewed changes

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch from 11b3c97 to da6dd02 Compare January 24, 2022 16:53

mossmaurice changed the title ~~Remove error handler calls from hoofs~~ iox-#590 Remove error handler calls from hoofs Jan 24, 2022

mossmaurice added 3 commits February 7, 2022 17:59

iox-eclipse-iceoryx#590 Templatize ErrorHandler and remove errorHandl…

8fd2cf4

…er() calls from hoofs Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#590 Update copyright headers

a697f00

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#590 Update doxygen of error handler

39824c1

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch from da6dd02 to 467b9f7 Compare February 7, 2022 17:18

mossmaurice commented Feb 7, 2022

View reviewed changes

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch 2 times, most recently from 6decac6 to 94ddf1c Compare February 7, 2022 17:36

iox-eclipse-iceoryx#590 Use a typed and untyped HandlerFunction to av…

cb654f2

…oid problems in tests Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch from 94ddf1c to cb654f2 Compare February 7, 2022 17:40

mossmaurice added 3 commits February 7, 2022 20:56

iox-eclipse-iceoryx#590 Use GTEST_FAIL instead of FAIL

873613c

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#590 Link iceoryx_hoofs_testing against iceoryx_po…

f3d922b

…sh_testing Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#590 Carve out ErrorHandlerMock from ErrorHandler

91454e4

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch 2 times, most recently from dbcb178 to 897edd3 Compare February 8, 2022 10:51

iox-eclipse-iceoryx#590 Try to fix include error on CI

1d21571

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch from 897edd3 to 1d21571 Compare February 8, 2022 11:01

dkroenke reviewed Feb 8, 2022

View reviewed changes

iceoryx_binding_c/test/moduletests/test_c2cpp_enum_translation.cpp Show resolved Hide resolved

mossmaurice requested review from MatthiasKillat and elBoberido February 8, 2022 13:06

iox-eclipse-iceoryx#590 Move ErrorHandlerMock to mock directory

69d9448

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#590-remove-error-handler-calls-from-hoofs branch from 78b56b5 to 69d9448 Compare February 8, 2022 13:12

elBoberido reviewed Feb 9, 2022

View reviewed changes

elfenpiff requested changes Feb 9, 2022

View reviewed changes

elfenpiff reviewed Feb 9, 2022

View reviewed changes

iceoryx_hoofs/testing/error_handler_mock.cpp Outdated Show resolved Hide resolved

MatthiasKillat reviewed Feb 9, 2022

View reviewed changes

mossmaurice added 3 commits February 12, 2022 22:20

iox-eclipse-iceoryx#590 Better output in case of failing test due to …

28823ae

…unexpected error enum type in error handler Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#590 Add copyright header to changed files

0c581ae

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#590 Move mock to correct dir and remove obsolete …

fd0b941

…todo Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice closed this Feb 12, 2022

mossmaurice mentioned this pull request Feb 12, 2022

iox-#1099 Separate test part from ErrorHandler and call errorHandler() in iceoryx_hoofs only once #1100

Merged

21 tasks

iox-#590 Remove error handler calls from hoofs #1027

iox-#590 Remove error handler calls from hoofs #1027

Conversation

mossmaurice commented Jan 19, 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

mossmaurice Feb 12, 2022 • edited Loading

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 Jan 19, 2022 • edited Loading

Codecov Report

elfenpiff left a comment

Choose a reason for hiding this comment

MatthiasKillat commented Jan 20, 2022 • edited Loading

mossmaurice commented Jan 20, 2022

elBoberido commented Jan 21, 2022

elBoberido left a 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

MatthiasKillat left a 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

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

MatthiasKillat commented Jan 21, 2022 • edited Loading

elfenpiff left a comment

Choose a reason for hiding this comment

mossmaurice commented Jan 24, 2022 • edited Loading

mossmaurice Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido commented Feb 7, 2022

mossmaurice commented Feb 8, 2022

elBoberido left a 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

MatthiasKillat Feb 9, 2022 • edited Loading

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

Choose a reason for hiding this comment

elfenpiff left a 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

mossmaurice commented Jan 19, 2022 •

edited

Loading

mossmaurice Feb 12, 2022 •

edited

Loading

codecov bot commented Jan 19, 2022 •

edited

Loading

MatthiasKillat commented Jan 20, 2022 •

edited

Loading

MatthiasKillat commented Jan 21, 2022 •

edited

Loading

mossmaurice commented Jan 24, 2022 •

edited

Loading

mossmaurice Feb 7, 2022 •

edited

Loading

MatthiasKillat Feb 9, 2022 •

edited

Loading

MatthiasKillat Feb 9, 2022 •

edited

Loading