You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2020/07/01 06:07:01 UTC

[GitHub] [celix] pnoltes opened a new pull request #265: Bugfix/pstm deadlock work outside of locks

pnoltes opened a new pull request #265:
URL: https://github.com/apache/celix/pull/265


   Fixes an issue in the pubsub topology manager (pstm), which can deadlocks when setting up topic senders.
   
   This PR is a continuation of #261. As result #261 can be closed.
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] codecov-commenter commented on pull request #265: Bugfix/pstm deadlock work outside of locks

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #265:
URL: https://github.com/apache/celix/pull/265#issuecomment-652376640


   # [Codecov](https://codecov.io/gh/apache/celix/pull/265?src=pr&el=h1) Report
   > Merging [#265](https://codecov.io/gh/apache/celix/pull/265?src=pr&el=desc) into [master](https://codecov.io/gh/apache/celix/commit/4ae05dac73bf2421d8a5eebc46fbeaf6cc22b8e9&el=desc) will **increase** coverage by `0.16%`.
   > The diff coverage is `66.03%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/celix/pull/265/graphs/tree.svg?width=650&height=150&src=pr&token=JdsiThga8P)](https://codecov.io/gh/apache/celix/pull/265?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #265      +/-   ##
   ==========================================
   + Coverage   67.94%   68.10%   +0.16%     
   ==========================================
     Files         129      136       +7     
     Lines       26696    27011     +315     
   ==========================================
   + Hits        18138    18397     +259     
   - Misses       8558     8614      +56     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/celix/pull/265?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libs/framework/src/dm\_dependency\_manager\_impl.c](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL2RtX2RlcGVuZGVuY3lfbWFuYWdlcl9pbXBsLmM=) | `33.53% <0.00%> (-0.21%)` | :arrow_down: |
   | [...sub\_topology\_manager/src/pubsub\_topology\_manager.c](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX3RvcG9sb2d5X21hbmFnZXIvc3JjL3B1YnN1Yl90b3BvbG9neV9tYW5hZ2VyLmM=) | `52.74% <66.66%> (+2.29%)` | :arrow_up: |
   | [libs/framework/include/celix/dm/Component\_Impl.h](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnRfSW1wbC5o) | `62.65% <0.00%> (ø)` | |
   | [...ramework/include/celix/dm/DependencyManager\_Impl.h](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9EZXBlbmRlbmN5TWFuYWdlcl9JbXBsLmg=) | `87.50% <0.00%> (ø)` | |
   | [libs/framework/include/celix/dm/types.h](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS90eXBlcy5o) | `92.85% <0.00%> (ø)` | |
   | [libs/framework/include/celix/dm/Component.h](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnQuaA==) | `100.00% <0.00%> (ø)` | |
   | [...ibs/framework/include/celix/dm/DependencyManager.h](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9EZXBlbmRlbmN5TWFuYWdlci5o) | `100.00% <0.00%> (ø)` | |
   | [...ibs/framework/include/celix/dm/ServiceDependency.h](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9TZXJ2aWNlRGVwZW5kZW5jeS5o) | `84.21% <0.00%> (ø)` | |
   | [...ramework/include/celix/dm/ServiceDependency\_Impl.h](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9TZXJ2aWNlRGVwZW5kZW5jeV9JbXBsLmg=) | `64.70% <0.00%> (ø)` | |
   | [libs/framework/src/framework.c](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL2ZyYW1ld29yay5j) | `75.44% <0.00%> (+0.14%)` | :arrow_up: |
   | ... and [6 more](https://codecov.io/gh/apache/celix/pull/265/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/265?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/265?src=pr&el=footer). Last update [4ae05da...74fa90c](https://codecov.io/gh/apache/celix/pull/265?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes commented on a change in pull request #265: Bugfix/pstm deadlock work outside of locks

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #265:
URL: https://github.com/apache/celix/pull/265#discussion_r448807194



##########
File path: libs/framework/src/dm_dependency_manager_impl.c
##########
@@ -173,7 +174,8 @@ static void celix_dm_getInfosCallback(void *handle, const celix_bundle_t *bnd) {
 
 celix_array_list_t * celix_dependencyManager_createInfos(celix_dependency_manager_t *manager) {
 	celix_array_list_t *infos = celix_arrayList_create();
-	celix_bundleContext_useBundles(manager->ctx, infos, celix_dm_getInfosCallback);
+	celix_framework_t* fw = celix_bundleContext_getFramework(manager->ctx);

Review comment:
       In de pubsub deadlock test the dependency manager is used from the framework bundle context. This works, but the 'dm' shell command was not printing info about the dm stuff of the framework. 
   This changes fixes that (using a useBundles call which includes the framework "bundle". 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] Oipo commented on pull request #265: Bugfix/pstm deadlock work outside of locks

Posted by GitBox <gi...@apache.org>.
Oipo commented on pull request #265:
URL: https://github.com/apache/celix/pull/265#issuecomment-652289921


   It seems that the pstm_deadlock_tcp_test still uses zmq somehow:
   
   ```
   WARNING: ThreadSanitizer: data race (pid=23761)
     Write of size 8 at 0x7b04000014b0 by thread T2 (mutexes: read M280, write M147, write M148, write M149):
       #0 socket <null> (libtsan.so.0+0x37f34)
       #1 <null> <null> (libzmq.so.5+0x6b5a8)
       #2 pubsub_zmqAdmin_setupTopicSender /home/oipo-unencrypted/Programming/celix-apache/bundles/pubsub/pubsub_admin_zmq/src/pubsub_zmq_admin.c:477 (libcelix_pubsub_admin_zmqd.so.1+0x2011a)
       #3 pstm_setupTopicSenderCallback /home/oipo-unencrypted/Programming/celix-apache/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c:886 (libcelix_pubsub_topology_managerd.so.1+0x1fd06)
       #4 serviceTracker_useHighestRankingServiceInternal /home/oipo-unencrypted/Programming/celix-apache/libs/framework/src/service_tracker.c:821 (libcelix_frameworkd.so.2+0xa2fb9)
       #5 celix_serviceTracker_useHighestRankingService /home/oipo-unencrypted/Programming/celix-apache/libs/framework/src/service_tracker.c:852 (libcelix_frameworkd.so.2+0xa33ac)
       #6 celix_bundleContext_useServiceWithOptions /home/oipo-unencrypted/Programming/celix-apache/libs/framework/src/bundle_context.c:784 (libcelix_frameworkd.so.2+0x6679d)
       #7 celix_bundleContext_useServiceWithId /home/oipo-unencrypted/Programming/celix-apache/libs/framework/src/bundle_context.c:731 (libcelix_frameworkd.so.2+0x65eca)
       #8 pstm_setupTopicSenders /home/oipo-unencrypted/Programming/celix-apache/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c:954 (libcelix_pubsub_topology_managerd.so.1+0x20bcf)
       #9 pstm_psaHandlingThread /home/oipo-unencrypted/Programming/celix-apache/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c:1104 (libcelix_pubsub_topology_managerd.so.1+0x23bbe)
       #10 <null> <null> (libtsan.so.0+0x2d1af)
   
     Previous write of size 8 at 0x7b04000014b0 by main thread:
       #0 pipe <null> (libtsan.so.0+0x3b8fc)
       #1 <null> <null> (libubsan.so.1+0x1d9f3)
       #2 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest.cc:2433 (pstm_deadlock_tcp_testd+0x195a81)
       #3 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest.cc:2469 (pstm_deadlock_tcp_testd+0x179d9e)
       #4 testing::TestInfo::Run() /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest.cc:2690 (pstm_deadlock_tcp_testd+0xfcbd2)
       #5 testing::TestSuite::Run() /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest.cc:2816 (pstm_deadlock_tcp_testd+0xffb55)
       #6 testing::internal::UnitTestImpl::RunAllTests() /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest.cc:5338 (pstm_deadlock_tcp_testd+0x12ab32)
       #7 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest.cc:2433 (pstm_deadlock_tcp_testd+0x19a7a8)
       #8 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest.cc:2469 (pstm_deadlock_tcp_testd+0x17e989)
       #9 testing::UnitTest::Run() /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest.cc:4925 (pstm_deadlock_tcp_testd+0x12026f)
       #10 RUN_ALL_TESTS() /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/include/gtest/gtest.h:2473 (pstm_deadlock_tcp_testd+0x1d2221)
       #11 main /home/oipo-unencrypted/Programming/celix-apache/build/_deps/googletest-src/googletest/src/gtest_main.cc:45 (pstm_deadlock_tcp_testd+0x1d2040)
   ```
   
   Ignore the "data race", as the race is introduced by the printf call that the sanitizer introduces. The important thing here is that zmq is used.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes commented on a change in pull request #265: Bugfix/pstm deadlock work outside of locks

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #265:
URL: https://github.com/apache/celix/pull/265#discussion_r448823326



##########
File path: libs/framework/src/dm_dependency_manager_impl.c
##########
@@ -173,7 +174,8 @@ static void celix_dm_getInfosCallback(void *handle, const celix_bundle_t *bnd) {
 
 celix_array_list_t * celix_dependencyManager_createInfos(celix_dependency_manager_t *manager) {
 	celix_array_list_t *infos = celix_arrayList_create();
-	celix_bundleContext_useBundles(manager->ctx, infos, celix_dm_getInfosCallback);
+	celix_framework_t* fw = celix_bundleContext_getFramework(manager->ctx);

Review comment:
       yes the dm is part of the framework. This removed the need for additional linking (per bundle) and more importantly the needed special linking instructions with the "-Wl,--no-as-needed" for linux and for osx something else. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes merged pull request #265: Bugfix/pstm deadlock work outside of locks

Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #265:
URL: https://github.com/apache/celix/pull/265


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] abroekhuis commented on a change in pull request #265: Bugfix/pstm deadlock work outside of locks

Posted by GitBox <gi...@apache.org>.
abroekhuis commented on a change in pull request #265:
URL: https://github.com/apache/celix/pull/265#discussion_r448800987



##########
File path: libs/framework/src/dm_dependency_manager_impl.c
##########
@@ -173,7 +174,8 @@ static void celix_dm_getInfosCallback(void *handle, const celix_bundle_t *bnd) {
 
 celix_array_list_t * celix_dependencyManager_createInfos(celix_dependency_manager_t *manager) {
 	celix_array_list_t *infos = celix_arrayList_create();
-	celix_bundleContext_useBundles(manager->ctx, infos, celix_dm_getInfosCallback);
+	celix_framework_t* fw = celix_bundleContext_getFramework(manager->ctx);

Review comment:
       Why this change?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] abroekhuis commented on a change in pull request #265: Bugfix/pstm deadlock work outside of locks

Posted by GitBox <gi...@apache.org>.
abroekhuis commented on a change in pull request #265:
URL: https://github.com/apache/celix/pull/265#discussion_r448807851



##########
File path: libs/framework/src/dm_dependency_manager_impl.c
##########
@@ -173,7 +174,8 @@ static void celix_dm_getInfosCallback(void *handle, const celix_bundle_t *bnd) {
 
 celix_array_list_t * celix_dependencyManager_createInfos(celix_dependency_manager_t *manager) {
 	celix_array_list_t *infos = celix_arrayList_create();
-	celix_bundleContext_useBundles(manager->ctx, infos, celix_dm_getInfosCallback);
+	celix_framework_t* fw = celix_bundleContext_getFramework(manager->ctx);

Review comment:
       Ok, the dm is explicitly part of the fw? Since this now added a direct include to the framework which was not needed previously.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org