Skip to content

Remove deadlock in config notification#1040

Merged
tcormackMW merged 5 commits intodevelopmentfrom
configNotifierDeadlockFix
Sep 23, 2024
Merged

Remove deadlock in config notification#1040
tcormackMW merged 5 commits intodevelopmentfrom
configNotifierDeadlockFix

Conversation

@tcormackMW
Copy link
Copy Markdown
Contributor

Removes the 'ordering lock' that was doing little other than causing Deadlocks

Before change, the added test case causes deadlock, after change it does not

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (070fd93) to head (f87e28f).
Report is 2 commits behind head on development.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1040      +/-   ##
===============================================
- Coverage        88.40%   88.39%   -0.01%     
===============================================
  Files              252      252              
  Lines            12156    12155       -1     
===============================================
- Hits             10746    10745       -1     
  Misses            1410     1410              
Files with missing lines Coverage Δ
...tiveServices/src/manager/ConfigurationNotifier.cpp 93.26% <ø> (+0.88%) ⬆️
...tiveServices/src/manager/ConfigurationNotifier.hpp 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment thread compendium/DeclarativeServices/test/TestUtils.cpp Outdated
Comment thread compendium/DeclarativeServices/test/TestUtils.cpp Outdated
Comment thread compendium/DeclarativeServices/test/TestUtils.hpp
Comment thread compendium/DeclarativeServices/test/gtest/TestAsyncWorkService.cpp
Comment thread compendium/DeclarativeServices/test/gtest/TestUpdateConfiguration.cpp Outdated
Comment thread compendium/DeclarativeServices/test/gtest/TestUpdateConfiguration.cpp Outdated
Comment thread compendium/test_bundles/TestBundleDSCA10/src/ServiceImpl.cpp Outdated
Comment thread compendium/test_bundles/TestBundleDSCA10/src/ServiceImpl.hpp
Comment thread compendium/test_bundles/TestBundleDSCA10/src/ServiceImpl.hpp
Comment thread compendium/test_bundles/TestBundleDSCA10/src/ServiceImpl.hpp
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment thread compendium/DeclarativeServices/test/TestUtils.cpp Outdated
Comment thread compendium/DeclarativeServices/test/gtest/TestAsyncWorkService.cpp
Comment thread compendium/DeclarativeServices/test/gtest/TestAsyncWorkService.cpp
Comment thread compendium/test_bundles/TestBundleDSCA10/src/ServiceImpl.cpp Outdated
Copy link
Copy Markdown
Contributor

@Burgch Burgch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks Toby!

Comment thread compendium/DeclarativeServices/test/TestUtils.cpp Outdated
Comment thread compendium/DeclarativeServices/test/gtest/TestAsyncWorkService.cpp Outdated
Comment thread compendium/test_bundles/TestBundleDSCA10/src/ServiceImpl.cpp
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment thread compendium/test_bundles/TestBundleDSCA10/src/ServiceImpl.cpp
Copy link
Copy Markdown
Member

@jeffdiclemente jeffdiclemente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once the other comments are addressed.

Comment thread compendium/DeclarativeServices/test/gtest/TestAsyncWorkService.cpp Outdated
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment thread compendium/DeclarativeServices/test/TestUtils.cpp
@tcormackMW tcormackMW requested a review from Burgch September 18, 2024 18:52
Copy link
Copy Markdown
Contributor

@Burgch Burgch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tcormackMW tcormackMW merged commit 2e4eca4 into development Sep 23, 2024
@tcormackMW tcormackMW deleted the configNotifierDeadlockFix branch September 23, 2024 13:38
RobertLauferElektrobit pushed a commit to insi-eb/CppMicroServices-cpp14 that referenced this pull request Jul 28, 2025
* added test to verify that update call from inside modified no longer causes deadlock

* memory leaks... no idea why they are happening but 99% sure this is a false positive

* fix false leak

* respond to jeff comments

* respond to conor
's comment

---------

Co-authored-by: Toby Cormack <tcormack@mathworks.com>
jeffdiclemente added a commit that referenced this pull request Jul 29, 2025
* added test to verify that update call from inside modified no longer causes deadlock

* memory leaks... no idea why they are happening but 99% sure this is a false positive

* fix false leak

* respond to jeff comments

* respond to conor
's comment

---------

Co-authored-by: tcormackMW <113473781+tcormackMW@users.noreply.github.com>
Co-authored-by: Toby Cormack <tcormack@mathworks.com>
Co-authored-by: Jeff DiClemente <jeffdiclemente@users.noreply.github.com>
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.

3 participants