Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/556 osgi uninstall #569

Merged
merged 23 commits into from
Jun 16, 2023
Merged

Feature/556 osgi uninstall #569

merged 23 commits into from
Jun 16, 2023

Conversation

PengZheng
Copy link
Contributor

@PengZheng PengZheng commented Jun 2, 2023

To fix #556, #557, part of #561, and #563, this PR introduces:

  • Per bundle "lifecycle" read-write lock. To change bundle state, the write lock must be held.
  • celix_framework_useActiveBundles, which holds the per bundle read lock when calling the provided use callback.
    The bundle state is guaranteed to be active during the callback. For other cases like celix_bundleContext_getBundleSymbolicName, such strong guarantee is not needed. Therefore, celix_framework_useBundles is still there.
    • Another interesting use case is observed in pubsub_topology_manager, a service/tracker owner's bundle object, which is tracked, is always safe to access. Calling celix_framework_useActiveBundles will lead to ABBA deadlock while calling celix_framework_useBundles will not. The test bundle (tst_activator.c) registers several subscribers synchronously, waiting the second registration to return (i.e. waiting for the event loop). The event loop is trying to add the second subscriber into topicReceivers, waiting for manager->topicReceivers.mutex. The owner of manager->topicReceivers.mutex is calling celix_framework_useActiveBundles, trying to hold test bundle's lifecycle read-lock. Unfortunately, the write lock is held before calling the test bundle's activator start.
      • Modifying the test bundle to do asynchronous registration works, but that will limit our users' freedom to perform synchronous registration.
  • uninstall of semantics similar to OSGi specs. It removes the bundle from the bundle cache and returns the bundle id to the framework.
  • unload operation, which is similar to uninstall but leaves the bundle in cache, so that next time reloaded from the cache, the bundle id does not change. To avoid any confusion, it is used only for testing purpose and is not exposed.
  • A coarse install lock, which essentially serialize install/uninstall/unload operations. Thus bundle id namespace and the bundle cache are kept in sync.
  • update implemented as unload plus install, avoid any bundle state inconsistency (Bundle state inconsistency in case of update failure #563) in case of update failure.
  • Some build system cleanup, including:

I plan to add thorough whitebox tests to the life cycle layer and the module layer after this merged and #522 implemented.

PengZheng added 8 commits May 31, 2023 16:55
It fixes #561, an ABBA deadlock in test, and a potential double-free.
Unloading a bundle from a framework instance will release all its resources but leave the bundle in cache so that it can be reloaded using `install`.
It fixes thread-safety issue in dependency manager (see #561), a null string logging issue in pubsub's topology manager, and a potential crash in query_command.c.
@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

Merging #569 (eec55b4) into master (ab2cc5e) will increase coverage by 0.20%.
The diff coverage is 80.84%.

❗ Current head eec55b4 differs from pull request most recent head 58a3c3b. Consider uploading reports for the commit 58a3c3b to get more accurate results

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage   78.08%   78.28%   +0.20%     
==========================================
  Files         229      230       +1     
  Lines       35016    35013       -3     
==========================================
+ Hits        27341    27409      +68     
+ Misses       7675     7604      -71     
Impacted Files Coverage Δ
...sub_topology_manager/src/pubsub_topology_manager.c 52.38% <0.00%> (-0.45%) ⬇️
libs/framework/src/bundle.c 72.01% <0.00%> (+2.35%) ⬆️
libs/framework/src/bundle_archive.c 100.00% <ø> (+3.37%) ⬆️
libs/framework/src/bundle_context.c 80.02% <25.00%> (-0.19%) ⬇️
libs/framework/src/framework.c 81.45% <82.35%> (+3.78%) ⬆️
...framework/src/framework_bundle_lifecycle_handler.c 86.27% <91.66%> (+30.60%) ⬆️
bundles/shell/shell/src/query_command.c 84.00% <100.00%> (ø)
bundles/shell/shell/src/std_commands.c 100.00% <100.00%> (ø)
bundles/shell/shell/src/unload_command.c 100.00% <100.00%> (ø)
libs/framework/src/bundle_revision.c 100.00% <100.00%> (+27.08%) ⬆️
... and 2 more

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@PengZheng PengZheng changed the title [WIP]Feature/556 osgi uninstall Feature/556 osgi uninstall Jun 3, 2023
@PengZheng PengZheng requested a review from pnoltes June 3, 2023 08:27
@PengZheng PengZheng linked an issue Jun 8, 2023 that may be closed by this pull request
@PengZheng
Copy link
Contributor Author

Could you help review this? @pnoltes
I intend to merge this before more work (and tests) on module/life cycle layer.

@pnoltes
Copy link
Contributor

pnoltes commented Jun 12, 2023

Could you help review this? @pnoltes I intend to merge this before more work (and tests) on module/life cycle layer.

Yes, happily. Please allow me a few days to find the time to thoroughly review this.

Copy link
Contributor

@pnoltes pnoltes left a comment

Choose a reason for hiding this comment

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

Nice changes and good to see the unload addition and that the update bundle is now possible.

I do have some remarks for the recursive mutex and that the read lock is only used with useActiveBundles

libs/framework/src/framework.c Show resolved Hide resolved
libs/framework/src/framework.c Outdated Show resolved Hide resolved
cmake/cmake_celix/BundlePackaging.cmake Show resolved Hide resolved
libs/framework/include/celix_bundle_context.h Outdated Show resolved Hide resolved
@PengZheng PengZheng requested a review from pnoltes June 16, 2023 03:12
Copy link
Contributor

@pnoltes pnoltes left a comment

Choose a reason for hiding this comment

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

LGTM.

Maybe the useActiveBundle doc can mention that the framework ensures that the bundle state cannot change during a callback.

@PengZheng PengZheng merged commit 24d624e into master Jun 16, 2023
@PengZheng PengZheng deleted the feature/556-osgi-uninstall branch June 16, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants