iox-#33 posix mutex support in windows #797

elfenpiff · 2021-05-17T17:02:24Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

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
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 Basic Windows 10 support #33

codecov · 2021-05-17T17:23:20Z

Codecov Report

Merging #797 (f812379) into master (92f5bde) will decrease coverage by 0.04%.
The diff coverage is 83.33%.

❗ Current head f812379 differs from pull request most recent head 5bbfb72. Consider uploading reports for the commit 5bbfb72 to get more accurate results

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
- Coverage   74.41%   74.36%   -0.05%     
==========================================
  Files         322      322              
  Lines       11522    11533      +11     
  Branches     1952     1956       +4     
==========================================
+ Hits         8574     8577       +3     
- Misses       2190     2193       +3     
- Partials      758      763       +5

Flag	Coverage Δ
unittests	`73.21% <83.33%> (-0.03%)`	⬇️
unittests_timing	`30.92% <41.66%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
iceoryx_hoofs/source/posix_wrapper/mutex.cpp	`81.25% <33.33%> (-5.42%)`	⬇️
...ceoryx_hoofs/internal/posix_wrapper/posix_call.inl	`100.00% <100.00%> (ø)`
iceoryx_hoofs/source/concurrent/loffli.cpp	`79.41% <0.00%> (-5.89%)`	⬇️
iceoryx_hoofs/source/posix_wrapper/timer.cpp	`60.60% <0.00%> (-0.87%)`	⬇️
...nternal/roudi/introspection/port_introspection.inl	`71.89% <0.00%> (-0.73%)`	⬇️

.github/workflows/build-test.yml

iceoryx_hoofs/test/moduletests/test_posix_mutex.cpp

.github/workflows/build-test.yml

iceoryx_examples/waitset/ice_waitset_basic.cpp

iceoryx_hoofs/cmake/IceoryxPlatform.cmake

iceoryx_hoofs/platform/win/include/iceoryx_hoofs/platform/pthread.hpp

iceoryx_hoofs/platform/win/source/semaphore.cpp

iceoryx_hoofs/test/moduletests/test_posix_mutex.cpp

iceoryx_hoofs/cmake/IceoryxPlatform.cmake

iceoryx_hoofs/source/posix_wrapper/mutex.cpp

iceoryx_hoofs/test/moduletests/test_posix_mutex.cpp

iceoryx_hoofs/test/moduletests/test_posix_posix_call.cpp

iceoryx_hoofs/test/moduletests/test_posix_mutex.cpp

…accordingly, fixed issue where the wrong errno was set in windows platform semaphore Signed-off-by: Christian Eltzschig <me@elchris.org>

Signed-off-by: Christian Eltzschig <me@elchris.org>

…posixCall assigns returnValue to errnum if it contains errno Signed-off-by: Christian Eltzschig <me@elchris.org>

…six call Signed-off-by: Christian Eltzschig <me@elchris.org>

Signed-off-by: Christian Eltzschig <me@elchris.org>

…ting time < 1, linking error fix in mutex posix Signed-off-by: Christian Eltzschig <me@elchris.org>

…ility of code, steady clock instead high resolution clock (since it is not the same on all platforms), added missing posix call tests Signed-off-by: Christian Eltzschig <me@elchris.org>

Signed-off-by: Christian Eltzschig <me@elchris.org>

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

MatthiasKillat · 2021-05-20T11:23:23Z

iceoryx_hoofs/test/moduletests/test_posix_mutex.cpp

@@ -34,7 +34,7 @@ class Mutex_test : public Test
    void SetUp() override
    {
        internal::CaptureStderr();
-        m_deadlockWatchdog.watchAndActOnFailure([] { std::terminate(); });
+        deadlockWatchdog.watchAndActOnFailure([] { std::terminate(); });


We could also ASSERT_TRUE(false) here instead of terminate. I have not tested how it behaves when called from another thread (like here in the watchdog) but maybe it would be possible to abort the current test this way with a failure and still run the next test (to get more info on what is broken).

If this does not work I see no easy way to abort only the current test (which is considered blocked).

Èdit: ASSERT_TRUE is a crude way of doing this, better use FAIL.

assert just return and does not unblock the deadlock

Right.We could in theory still do this in another way by essentially creating a NO_DEATH test which under normal conditions expects not to terminate but can terminated like here due to timeout. I did something like this some time ago (not with timeout) in the C binding with an invalid options test case, Gtest has the necessary macro infrastructure.

This would work (I think) because those are forked and if we terminate the forked test it would fail and a blocked mutex is of no concern since thre process is terminated anyway.

But it is not worth it to do so, I just think it is possible in principle.

dkroenke · 2021-05-20T11:45:02Z

.clang-tidy

@@ -7,7 +7,8 @@ CheckOptions:
  - { key: readability-identifier-naming.MethodCase,              value: camelBack }
  - { key: readability-identifier-naming.FunctionCase,            value: camelBack }
  - { key: readability-identifier-naming.NamespaceCase,           value: lower_case }
-  - { key: readability-identifier-naming.MemberPrefix,            value: m_ }
+  - { key: readability-identifier-naming.PrivateMemberPrefix,     value: m_ }


To which clang-tidy version refers this change?

@dkroenke This should be available in clang-tidy 10, see: https://releases.llvm.org/10.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-identifier-naming.html

At the moment I would not yet enforce this clang-tidy file since needs to mature a little while longer. But I strongly suggest that we all integrate clang-tidy into our IDE so that this file can mature faster.

elfenpiff requested review from mossmaurice and dkroenke May 17, 2021 17:30

elfenpiff marked this pull request as ready for review May 17, 2021 17:30

dkroenke reviewed May 18, 2021

View reviewed changes

.github/workflows/build-test.yml Show resolved Hide resolved

.github/workflows/build-test.yml Outdated Show resolved Hide resolved

iceoryx_hoofs/test/moduletests/test_posix_mutex.cpp Show resolved Hide resolved

elfenpiff requested a review from elBoberido May 18, 2021 10:31

elBoberido reviewed May 18, 2021

View reviewed changes

mossmaurice assigned elfenpiff May 18, 2021

mossmaurice added the enhancement New feature label May 18, 2021

elfenpiff requested review from elBoberido and dkroenke May 19, 2021 15:38

elBoberido reviewed May 19, 2021

View reviewed changes

iceoryx_hoofs/test/moduletests/test_posix_mutex.cpp Outdated Show resolved Hide resolved

elfenpiff requested a review from elBoberido May 19, 2021 19:45

elfenpiff and others added 11 commits May 20, 2021 12:17

iox-#33 under windows the mutex is always recursive - adjusted tests …

1dea212

…accordingly, fixed issue where the wrong errno was set in windows platform semaphore Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 fixed copyright header

7866a24

Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 introduce line breaks for better readability

d94af47

Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 revert line breaks in build yaml

ab7d04b

Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 refactored mutex tests to be more performant with less wait, …

bf2b80a

…posixCall assigns returnValue to errnum if it contains errno Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 implement unit tests when errno is set via return value of po…

0ce94c6

…six call Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 remove unused argument

cb3c12a

Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 adjusted waiting time calculation to always round up when wai…

f5b6450

…ting time < 1, linking error fix in mutex posix Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 m_ only for non public members in clang-tidy, improved readab…

942e565

…ility of code, steady clock instead high resolution clock (since it is not the same on all platforms), added missing posix call tests Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 removed superfluous additional millisecond cast

cd0f3cb

Signed-off-by: Christian Eltzschig <me@elchris.org>

iox-#33 capture by reference in lambdas to fix Windows build

5bbfb72

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

elfenpiff force-pushed the iox-#33-recursive-mutex branch from f812379 to 5bbfb72 Compare May 20, 2021 10:17

elBoberido approved these changes May 20, 2021

View reviewed changes

MatthiasKillat reviewed May 20, 2021

View reviewed changes

dkroenke reviewed May 20, 2021

View reviewed changes

dkroenke approved these changes May 20, 2021

View reviewed changes

elfenpiff merged commit d3f697f into eclipse-iceoryx:master May 20, 2021

elfenpiff deleted the iox-#33-recursive-mutex branch May 20, 2021 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#33 posix mutex support in windows #797

iox-#33 posix mutex support in windows #797

elfenpiff commented May 17, 2021 •

edited

Loading

codecov bot commented May 17, 2021 •

edited

Loading

MatthiasKillat May 20, 2021 •

edited

Loading

elBoberido May 20, 2021

MatthiasKillat May 20, 2021

dkroenke May 20, 2021

elfenpiff May 20, 2021

iox-#33 posix mutex support in windows #797

iox-#33 posix mutex support in windows #797

Conversation

elfenpiff commented May 17, 2021 • 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

codecov bot commented May 17, 2021 • edited Loading

Codecov Report

MatthiasKillat May 20, 2021 • edited Loading

Choose a reason for hiding this comment

elBoberido May 20, 2021

Choose a reason for hiding this comment

MatthiasKillat May 20, 2021

Choose a reason for hiding this comment

dkroenke May 20, 2021

Choose a reason for hiding this comment

elfenpiff May 20, 2021

Choose a reason for hiding this comment

elfenpiff commented May 17, 2021 •

edited

Loading

codecov bot commented May 17, 2021 •

edited

Loading

MatthiasKillat May 20, 2021 •

edited

Loading