Hotfix/The Correct Bundle Update behavior

libs/framework/gtest/src/BundleArchiveWithErrorInjectionTestSuite.cc

@@ -198,7 +198,7 @@ TEST_F(CelixBundleArchiveErrorInjectionTestSuite, ArchiveCreateErrorTest) {
     EXPECT_FALSE(celix_utils_directoryExists(TEST_ARCHIVE_ROOT));
     teardownErrorInjectors();
 
-    celix_ei_expect_lstat((void*) celix_bundleArchive_create, 2, -1);
+    celix_ei_expect_lstat((void*) celix_bundleArchive_create, 3, -1);
     EXPECT_EQ(CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,EACCES),
               celix_bundleArchive_create(&fw, TEST_ARCHIVE_ROOT, 1, SIMPLE_TEST_BUNDLE1_LOCATION, &archive));
     EXPECT_EQ(nullptr, archive);
@@ -213,7 +213,7 @@ TEST_F(CelixBundleArchiveErrorInjectionTestSuite, ArchiveCreateErrorTest) {
     archive = nullptr;
     std::this_thread::sleep_for(std::chrono::milliseconds(10));
     celix_utils_touch(SIMPLE_TEST_BUNDLE1_LOCATION);
-    celix_ei_expect_unlink((void*)celix_bundleArchive_create, 2, -1);
+    celix_ei_expect_unlink((void*)celix_bundleArchive_create, 3, -1);
     EXPECT_EQ(CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,EACCES),
               celix_bundleArchive_create(&fw, TEST_ARCHIVE_ROOT, 2, SIMPLE_TEST_BUNDLE1_LOCATION, &archive));
     EXPECT_EQ(nullptr, archive);
@@ -226,7 +226,7 @@ TEST_F(CelixBundleArchiveErrorInjectionTestSuite, ArchiveCreateErrorTest) {
     archive = nullptr;
     std::this_thread::sleep_for(std::chrono::milliseconds(10));
     celix_utils_touch(SIMPLE_TEST_BUNDLE1_LOCATION);
-    celix_ei_expect_celix_utils_deleteDirectory((void*)celix_bundleArchive_create, 2, CELIX_FILE_IO_EXCEPTION);
+    celix_ei_expect_celix_utils_deleteDirectory((void*)celix_bundleArchive_create, 3, CELIX_FILE_IO_EXCEPTION);
     EXPECT_EQ(CELIX_FILE_IO_EXCEPTION,
               celix_bundleArchive_create(&fw, TEST_ARCHIVE_ROOT, 1, SIMPLE_TEST_BUNDLE1_LOCATION, &archive));
     EXPECT_EQ(nullptr, archive);

libs/framework/gtest/src/CelixBundleCacheErrorInjectionTestSuite.cc

@@ -147,18 +147,6 @@ TEST_F(CelixBundleCacheErrorInjectionTestSuite, SystemArchiveCreateErrorTest) {
     EXPECT_EQ(CELIX_SUCCESS, celix_bundleCache_destroy(cache));
 }
 
-TEST_F(CelixBundleCacheErrorInjectionTestSuite, ArchiveDestroyErrorTest) {
-    celix_bundle_cache_t* cache = nullptr;
-    createCache(&cache);
-    bundle_archive_t* archive = nullptr;
-    EXPECT_EQ(CELIX_SUCCESS, celix_bundleCache_createArchive(cache, 1, SIMPLE_TEST_BUNDLE1_LOCATION, &archive));
-    celix_ei_expect_celix_utils_deleteDirectory((void*)celix_bundleCache_destroyArchive, 1, CELIX_FILE_IO_EXCEPTION);
-    std::string storeRoot = celix_bundleArchive_getPersistentStoreRoot(archive);
-    EXPECT_EQ(CELIX_FILE_IO_EXCEPTION, celix_bundleCache_destroyArchive(cache, archive, true));
-    EXPECT_TRUE(celix_utils_directoryExists(storeRoot.c_str()));
-    EXPECT_EQ(CELIX_SUCCESS, celix_bundleCache_destroy(cache));
-}
-
 TEST_F(CelixBundleCacheErrorInjectionTestSuite, CreateBundleArchivesCacheErrorTest) {
     celix_bundle_cache_t* cache = nullptr;
     celix_properties_set(fw.configurationMap, CELIX_AUTO_START_1, SIMPLE_TEST_BUNDLE1_LOCATION);

libs/framework/gtest/src/CelixBundleCacheTestSuite.cc

@@ -58,7 +58,8 @@ TEST_F(CelixBundleCacheTestSuite, ArchiveCreateDestroyTest) {
     EXPECT_TRUE(celix_bundleCache_isBundleIdAlreadyUsed(fw.cache, 1));
     std::string loc = celix_bundleArchive_getPersistentStoreRoot(archive);
     EXPECT_TRUE(celix_utils_directoryExists(loc.c_str()));
-    EXPECT_EQ(CELIX_SUCCESS, celix_bundleCache_destroyArchive(fw.cache, archive, true));
+    celix_bundleArchive_invalidate(archive);
+    celix_bundleCache_destroyArchive(fw.cache, archive);
     EXPECT_EQ(-1, celix_bundleCache_findBundleIdForLocation(fw.cache, SIMPLE_TEST_BUNDLE1_LOCATION));
     EXPECT_FALSE(celix_bundleCache_isBundleIdAlreadyUsed(fw.cache, 1));
     EXPECT_FALSE(celix_utils_directoryExists(loc.c_str()));
@@ -70,7 +71,7 @@ TEST_F(CelixBundleCacheTestSuite, NonPermanentDestroyTest) {
     EXPECT_NE(nullptr, archive);
     std::string loc = celix_bundleArchive_getPersistentStoreRoot(archive);
     EXPECT_TRUE(celix_utils_directoryExists(loc.c_str()));
-    EXPECT_EQ(CELIX_SUCCESS, celix_bundleCache_destroyArchive(fw.cache, archive, false));
+    celix_bundleCache_destroyArchive(fw.cache, archive);
     EXPECT_EQ(1, celix_bundleCache_findBundleIdForLocation(fw.cache, SIMPLE_TEST_BUNDLE1_LOCATION));
     EXPECT_TRUE(celix_bundleCache_isBundleIdAlreadyUsed(fw.cache, 1));
     EXPECT_TRUE(celix_utils_directoryExists(loc.c_str()));
@@ -85,7 +86,8 @@ TEST_F(CelixBundleCacheTestSuite, SystemArchiveCreateDestroyTest) {
     EXPECT_EQ(CELIX_SUCCESS, bundleArchive_getArchiveRoot(archive, &archiveRoot));
     EXPECT_EQ(nullptr, archiveRoot);
     EXPECT_EQ(nullptr, celix_bundleArchive_getLocation(archive));
-    EXPECT_EQ(CELIX_SUCCESS, celix_bundleCache_destroyArchive(fw.cache, archive, true));
+    celix_bundleArchive_invalidate(archive);
+    celix_bundleCache_destroyArchive(fw.cache, archive);
 }
 
 TEST_F(CelixBundleCacheTestSuite, CreateBundleArchivesCacheTest) {

libs/framework/gtest/src/CelixBundleContextBundlesTestSuite.cc

@@ -29,6 +29,9 @@
 
 #include "bundle_context_private.h"
 #include "celix_api.h"
+#include "celix_bundle.h"
+#include "celix_bundle_context.h"
+#include "celix_stdlib_cleanup.h"
 #include "celix_file_utils.h"
 
 class CelixBundleContextBundlesTestSuite : public ::testing::Test {
@@ -313,6 +316,24 @@ TEST_F(CelixBundleContextBundlesTestSuite, UpdateBundlesTest) {
     ASSERT_FALSE(celix_bundleContext_updateBundle(ctx, bndId1, NULL));
 }
 
+TEST_F(CelixBundleContextBundlesTestSuite, ForceUpdateUsingBundleFromDifferentLocation) {
+    long bndId1 = celix_bundleContext_installBundle(ctx, TEST_BND1_LOC, true);
+    ASSERT_TRUE(bndId1 >= 0L);
+    ASSERT_TRUE(celix_bundleContext_isBundleInstalled(ctx, bndId1));
+    ASSERT_TRUE(celix_bundleContext_isBundleActive(ctx, bndId1));
+    celix_autofree char* name1 = celix_bundleContext_getBundleSymbolicName(ctx, bndId1);
+    EXPECT_TRUE(celix_bundleContext_useBundle(ctx, bndId1, nullptr, [] (void *, const bundle_t *bundle) {
+        // make bundle cache root newer than the bundle at location TEST_BND2_LOC
+        celix_autofree char* root = celix_bundle_getEntry(bundle, "/");
+        celix_utils_touch(root);
+    }));
+    ASSERT_TRUE(celix_bundleContext_updateBundle(ctx, bndId1, TEST_BND2_LOC));
+    ASSERT_TRUE(celix_bundleContext_isBundleInstalled(ctx, bndId1));
+    celix_autofree char* name2 = celix_bundleContext_getBundleSymbolicName(ctx, bndId1);
+    // bundle cache contains the bundle at location TEST_BND2_LOC
+    ASSERT_STRNE(name1, name2);
+}
+
 TEST_F(CelixBundleContextBundlesTestSuite, UpdateCorruptUncompressedBundlesTest) {
     const char* testExtractDir1 = "extractBundleTestDir1";
     const char* testExtractDir2 = "extractBundleTestDir2";

libs/framework/src/bundle.c

@@ -334,20 +334,8 @@ celix_status_t bundle_close(const_bundle_pt bundle) {
 }
 
 celix_status_t bundle_closeAndDelete(const_bundle_pt bundle) {
-	celix_status_t status;
-
-	bundle_archive_pt archive = NULL;
-
-    bundle_closeModules(bundle);
-    bundle_closeRevisions(bundle);
-    status = bundle_getArchive(bundle, &archive);
-    if (status == CELIX_SUCCESS) {
-    	bundleArchive_closeAndDelete(archive);
-    }
-
-	framework_logIfError(bundle->framework->logger, status, NULL, "Failed to close and delete bundle");
-
-    return status;
+    fw_log(bundle->framework->logger, CELIX_LOG_LEVEL_DEBUG, "Usage of bundle_closeAndDelete is deprecated and no longer needed. Called for bundle %s", bundle->symbolicName);
+    return CELIX_SUCCESS;
 }
 
 celix_status_t bundle_closeRevisions(const_bundle_pt bundle) {

libs/framework/src/bundle_archive.c

@@ -54,10 +54,10 @@ struct bundleArchive {
     char* resourceCacheRoot;
     char* bundleSymbolicName; // read from the manifest
     char* bundleVersion;      // read from the manifest
-
-    celix_thread_mutex_t lock;   // protects below and saving of bundle state properties
     bundle_revision_t* revision; // the current revision
     char* location;
+    bool cacheValid; // is the cache valid (e.g. not deleted)
+    bool valid; // is the archive valid (e.g. not deleted)
 };
 
 static celix_status_t celix_bundleArchive_storeBundleStateProperties(bundle_archive_pt archive) {
@@ -97,6 +97,35 @@ static celix_status_t celix_bundleArchive_storeBundleStateProperties(bundle_arch
     return CELIX_SUCCESS;
 }
 
+static celix_status_t celix_bundleArchive_removeResourceCache(bundle_archive_t* archive) {
+    celix_status_t status = CELIX_SUCCESS;
+    const char* error = NULL;
+    struct stat st;
+    status = lstat(archive->resourceCacheRoot, &st);
+    if (status == -1 && errno == ENOENT) {
+        return CELIX_SUCCESS;
+    } else if(status == -1 && errno != ENOENT) {
+        status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
+        fw_logCode(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, status, "Failed to stat bundle archive directory '%s'", archive->resourceCacheRoot);
+        return status;
+    }
+    assert(status == 0);
+    // celix_utils_deleteDirectory does not work for dangling symlinks, so handle this case separately
+    if (S_ISLNK(st.st_mode)) {
+        status = unlink(archive->resourceCacheRoot);
+        if (status == -1) {
+            status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
+            error = "Failed to remove existing bundle symlink";
+        }
+    } else {
+        status = celix_utils_deleteDirectory(archive->resourceCacheRoot, &error);
+    }
+    if (status != CELIX_SUCCESS) {
+        fw_logCode(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, status, "Failed to remove existing bundle archive revision directory '%s': %s", archive->resourceCacheRoot, error);
+    }
+    return status;
+}
+
 static celix_status_t
 celix_bundleArchive_extractBundle(bundle_archive_t* archive, const char* bundleUrl) {
     celix_status_t status = CELIX_SUCCESS;
@@ -126,30 +155,10 @@ celix_bundleArchive_extractBundle(bundle_archive_t* archive, const char* bundleU
      * If dlopen/dlsym is used with newer files, but with the same inode already used in dlopen/dlsym this leads to
      * segfaults.
      */
-    const char* error = NULL;
-    struct stat st;
-    status = lstat(archive->resourceCacheRoot, &st);
-    if(status == -1 && errno != ENOENT) {
-        status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
-        fw_logCode(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, status, "Failed to stat bundle archive directory '%s'", archive->resourceCacheRoot);
+    status = celix_bundleArchive_removeResourceCache(archive);
+    if (status != CELIX_SUCCESS) {
         return status;
     }
-    if (status == 0) {
-        // celix_utils_deleteDirectory does not work for dangling symlinks, so handle this case separately
-        if (S_ISLNK(st.st_mode)) {
-            status = unlink(archive->resourceCacheRoot);
-            if (status == -1) {
-                status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
-                error = "Failed to remove existing bundle symlink";
-            }
-        } else {
-            status = celix_utils_deleteDirectory(archive->resourceCacheRoot, &error);
-        }
-        if (status != CELIX_SUCCESS) {
-            fw_logCode(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, status, "Failed to remove existing bundle archive revision directory '%s': %s", archive->resourceCacheRoot, error);
-            return status;
-        }
-    }
     status = celix_framework_utils_extractBundle(archive->fw, bundleUrl, archive->resourceCacheRoot);
     if (status != CELIX_SUCCESS) {
         fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to initialize archive. Failed to extract bundle zip to revision directory.");
@@ -235,7 +244,6 @@ celix_status_t celix_bundleArchive_create(celix_framework_t* fw, const char *arc
 
     archive->fw = fw;
     archive->id = id;
-    celixThreadMutex_create(&archive->lock, NULL);
 
     if (isSystemBundle) {
         archive->resourceCacheRoot = getcwd(NULL, 0);
@@ -296,7 +304,8 @@ celix_status_t celix_bundleArchive_create(celix_framework_t* fw, const char *arc
             goto store_prop_failed;
         }
     }
-
+    archive->cacheValid = true;
+    archive->valid = true;
     *bundle_archive = archive;
     return CELIX_SUCCESS;
 store_prop_failed:
@@ -312,7 +321,7 @@ celix_status_t celix_bundleArchive_create(celix_framework_t* fw, const char *arc
     return status;
 }
 
-celix_status_t bundleArchive_destroy(bundle_archive_pt archive) {
+void bundleArchive_destroy(bundle_archive_pt archive) {
     if (archive != NULL) {
         free(archive->location);
         free(archive->savedBundleStatePropertiesPath);
@@ -322,10 +331,8 @@ celix_status_t bundleArchive_destroy(bundle_archive_pt archive) {
         free(archive->bundleSymbolicName);
         free(archive->bundleVersion);
         bundleRevision_destroy(archive->revision);
-        celixThreadMutex_destroy(&archive->lock);
         free(archive);
     }
-    return CELIX_SUCCESS;
 }
 
 celix_status_t bundleArchive_getId(bundle_archive_pt archive, long* id) {
@@ -342,19 +349,15 @@ const char* celix_bundleArchive_getSymbolicName(bundle_archive_pt archive) {
 }
 
 celix_status_t bundleArchive_getLocation(bundle_archive_pt archive, const char **location) {
-    celixThreadMutex_lock(&archive->lock);
     *location = archive->location;
-    celixThreadMutex_unlock(&archive->lock);
     return CELIX_SUCCESS;
 }
 
 char* celix_bundleArchive_getLocation(bundle_archive_pt archive) {
     char* result = NULL;
-    celixThreadMutex_lock(&archive->lock);
     if (archive->location) {
         result = celix_utils_strdup(archive->location);
     }
-    celixThreadMutex_unlock(&archive->lock);
     return result;
 }
 
@@ -371,9 +374,7 @@ celix_status_t bundleArchive_getCurrentRevisionNumber(bundle_archive_pt archive,
 //LCOV_EXCL_STOP
 
 celix_status_t bundleArchive_getCurrentRevision(bundle_archive_pt archive, bundle_revision_pt* revision) {
-    celixThreadMutex_lock(&archive->lock);
     *revision = archive->revision;
-    celixThreadMutex_unlock(&archive->lock);
     return CELIX_SUCCESS;
 }
 
@@ -416,9 +417,7 @@ celix_status_t bundleArchive_getLastModified(bundle_archive_pt archive, time_t*
 
 celix_status_t celix_bundleArchive_getLastModified(bundle_archive_pt archive, struct timespec* lastModified) {
     celix_status_t status;
-    celixThreadMutex_lock(&archive->lock);
     status = celix_utils_getLastModified(archive->resourceCacheRoot, lastModified);
-    celixThreadMutex_unlock(&archive->lock);
     return status;
 }
 
@@ -447,17 +446,15 @@ celix_status_t bundleArchive_close(bundle_archive_pt archive) {
     // not yet needed/possible
     return CELIX_SUCCESS;
 }
-//LCOV_EXCL_STOP
 
 celix_status_t bundleArchive_closeAndDelete(bundle_archive_pt archive) {
-    celix_status_t status = CELIX_SUCCESS;
-    if (archive->id != CELIX_FRAMEWORK_BUNDLE_ID) {
-        const char* err = NULL;
-        status = celix_utils_deleteDirectory(archive->archiveRoot, &err);
-        framework_logIfError(archive->fw->logger, status, NULL, "Failed to delete archive root '%s': %s", archive->archiveRoot, err);
-    }
-    return status;
+    fw_log(archive->fw->logger,
+           CELIX_LOG_LEVEL_DEBUG,
+           "Usage of bundleArchive_closeAndDelete is deprecated and no longer needed. Called for bundle %s",
+           archive->bundleSymbolicName);
+    return CELIX_SUCCESS;
 }
+//LCOV_EXCL_STOP
 
 const char* celix_bundleArchive_getPersistentStoreRoot(bundle_archive_t* archive) {
     return archive->storeRoot;
@@ -468,3 +465,29 @@ const char* celix_bundleArchive_getCurrentRevisionRoot(bundle_archive_t* archive
 }
 
 
+void celix_bundleArchive_invalidate(bundle_archive_pt archive) {
+    archive->valid = false;
+    archive->cacheValid = false;
+}
+
+void celix_bundleArchive_invalidateCache(bundle_archive_pt archive) {
+    archive->cacheValid = false;
+}
+
+bool celix_bundleArchive_isCacheValid(bundle_archive_pt archive) {
+    return archive->cacheValid;
+}
+
+void celix_bundleArchive_removeInvalidDirs(bundle_archive_pt archive) {
+    if (archive->id == CELIX_FRAMEWORK_BUNDLE_ID) {
+        return;
+    }
+    if (!archive->valid) {
+        celix_status_t status = CELIX_SUCCESS;
+        const char* err = NULL;
+        status = celix_utils_deleteDirectory(archive->archiveRoot, &err);
+        framework_logIfError(archive->fw->logger, status, NULL, "Failed to remove invalid archive root '%s': %s", archive->archiveRoot, err);
+    } else if (!archive->cacheValid){
+        (void)celix_bundleArchive_removeResourceCache(archive);
+    }
+}

libs/framework/src/bundle_archive_private.h

@@ -49,7 +49,7 @@ extern "C" {
  */
 celix_status_t celix_bundleArchive_create(celix_framework_t* fw, const char *archiveRoot, long id, const char *location, bundle_archive_pt *bundle_archive);
 
-celix_status_t bundleArchive_destroy(bundle_archive_pt archive);
+void bundleArchive_destroy(bundle_archive_pt archive);
 
 /**
  * @brief Returns the bundle id of the bundle archive.
@@ -75,6 +75,26 @@ const char* celix_bundleArchive_getPersistentStoreRoot(bundle_archive_t *archive
   */
 const char* celix_bundleArchive_getCurrentRevisionRoot(bundle_archive_pt archive);
 
+/**
+ * @brief Invalidate the whole bundle archive.
+ */
+void celix_bundleArchive_invalidate(bundle_archive_pt archive);
+
+/**
+ * @brief Invalidate the bundle archive's bundle cache.
+ */
+void celix_bundleArchive_invalidateCache(bundle_archive_pt archive);
+
+/**
+ * @brief Return if the bundle cache is valid.
+ */
+bool celix_bundleArchive_isCacheValid(bundle_archive_pt archive);
+
+/**
+ * @brief Remove all valid directories of the bundle archive.
+ */
+void celix_bundleArchive_removeInvalidDirs(bundle_archive_pt archive);
+
 #ifdef __cplusplus
 }
 #endif