Packages: fix circular refernece bug

Packages had a strong back-pointer to their catalog, creating a circular
reference loop. Break this so catalogs & packages are freed on exit.

simgear/io/HTTPRepository.cxx

@@ -417,7 +417,7 @@ public:
             SG_LOG(SG_TERRASYNC, SG_WARN, "updated file but not found in dir:" << _relativePath << " " << file);
         } else {
             if (it->hash != hash) {
-                SG_LOG(SG_TERRASYNC, SG_INFO, "Checksum error for " << absolutePath() << "/" << file << " " << it->hash << " " << hash);
+                SG_LOG(SG_TERRASYNC, SG_WARN, "Checksum error for " << absolutePath() << "/" << file << " " << it->hash << " " << hash);
                 // we don't erase the file on a hash mismatch, because if we're syncing during the
                 // middle of a server-side update, the downloaded file may actually become valid.
                 _repository->failedToUpdateChild(_relativePath, HTTPRepository::REPO_ERROR_CHECKSUM);
@@ -1209,15 +1209,15 @@ HTTPRepository::failure() const
         } else if (fileStatus == HTTPRepository::REPO_ERROR_CANCELLED) {
             // if we were cancelled, don't report or log
             return;
+        } else {
+            SG_LOG(SG_TERRASYNC, SG_WARN, "failed to update entry:" << relativePath << " status/code: "
+               << innerResultCodeAsString(fileStatus) << "/" << fileStatus);
         }
 
         Failure f;
         f.path = relativePath;
         f.error = fileStatus;
         failures.push_back(f);
-
-        SG_LOG(SG_TERRASYNC, SG_WARN, "failed to update entry:" << relativePath << " status/code: "
-               << innerResultCodeAsString(fileStatus) << "/" << fileStatus);
     }
 
 } // of namespace simgear

simgear/io/HTTPRequest.cxx

@@ -193,13 +193,16 @@ void Request::onDone()
 //------------------------------------------------------------------------------
 void Request::onFail()
 {
-  SG_LOG
-  (
-    SG_IO,
-    SG_INFO,
-    "request failed:" << url() << " : "
-                      << responseCode() << "/" << responseReason()
-  );
+  // log if we FAIELD< but not if we CANCELLED
+  if (_ready_state == FAILED) {
+    SG_LOG
+    (
+      SG_IO,
+      SG_INFO,
+      "request failed:" << url() << " : "
+                        << responseCode() << "/" << responseReason()
+    );
+  }
 }
 
 //------------------------------------------------------------------------------
@@ -344,12 +347,17 @@ void Request::setSuccess(int code)
 //------------------------------------------------------------------------------
 void Request::setFailure(int code, const std::string& reason)
 {
+  // we use -1 for cancellation, don't be noisy in that case
+  if (code >= 0) { 
     SG_LOG(SG_IO, SG_WARN, "HTTP request: set failure:" << code << " reason " << reason);
+  }
+
   _responseStatus = code;
   _responseReason = reason;
 
-  if( !isComplete() )
-    setReadyState(FAILED);
+  if( !isComplete() ) {
+    setReadyState(code < 0 ? CANCELLED : FAILED);
+  }
 }
 
 //------------------------------------------------------------------------------
@@ -373,6 +381,12 @@ void Request::setReadyState(ReadyState state)
 
     _cb_fail(this);
   }
+  else if (state == CANCELLED )
+  {
+    onFail(); // do this for compatability
+    onAlways();
+    _cb_fail(this);
+  }
   else
     return;
 
@@ -402,7 +416,7 @@ bool Request::serverSupportsPipelining() const
 //------------------------------------------------------------------------------
 bool Request::isComplete() const
 {
-  return _ready_state == DONE || _ready_state == FAILED;
+  return _ready_state == DONE || _ready_state == FAILED || _ready_state == CANCELLED;
 }
 
 //------------------------------------------------------------------------------

simgear/io/HTTPRequest.hxx

@@ -54,7 +54,8 @@ public:
       HEADERS_RECEIVED,
       LOADING,
       DONE,
-      FAILED
+      FAILED,
+      CANCELLED
     };
 
     virtual ~Request();

simgear/package/Catalog.cxx

@@ -595,13 +595,17 @@ void Catalog::setUserEnabled(bool b)
 
     m_userEnabled = b;
     SGPath disableMarkerFile = installRoot() / "_disabled_";
-    if (m_userEnabled) {
-        sg_ofstream of(disableMarkerFile);
-        of << "1\n"; // touch the file
+
+    if (m_userEnabled == false) {
+        sg_ofstream of(disableMarkerFile, std::ios::trunc | std::ios::out);
+        of << "1" << std::flush; // touch the file
+        of.close();
     } else {
-        bool ok = disableMarkerFile.remove();
-        if (!ok) {
-            SG_LOG(SG_GENERAL, SG_ALERT, "Failed to remove catalog-disable marker file:" << disableMarkerFile);
+        if (disableMarkerFile.exists()) {
+            const bool ok = disableMarkerFile.remove();
+            if (!ok) {
+                SG_LOG(SG_IO, SG_WARN, "Failed to remove catalog-disable marker file:" << disableMarkerFile);
+            }
         }
     }
 

simgear/package/Package.cxx

@@ -34,7 +34,7 @@ namespace simgear {
 namespace pkg {
 
 Package::Package(const SGPropertyNode* aProps, CatalogRef aCatalog) :
-    m_catalog(aCatalog)
+    m_catalog(aCatalog.get())
 {
     initWithProps(aProps);
 }
@@ -235,6 +235,11 @@ std::string Package::id() const
     return m_id;
 }
 
+CatalogRef Package::catalog() const
+{
+    return {m_catalog};
+}
+
 std::string Package::qualifiedId() const
 {
     return m_catalog->id() + "." + id();

simgear/package/Package.hxx

@@ -130,8 +130,7 @@ public:
 
     size_t fileSizeBytes() const;
 
-    CatalogRef catalog() const
-        { return m_catalog; }
+    CatalogRef catalog() const;
 
     bool matches(const SGPropertyNode* aFilter) const;
 
@@ -236,7 +235,7 @@ private:
     SGPropertyNode_ptr m_props;
     std::string m_id;
     string_set m_tags;
-    CatalogRef m_catalog;
+    Catalog* m_catalog = nullptr; // non-owning ref, explicitly
     string_list m_variants;
 
     mutable function_list<InstallCallback> _install_cb;

simgear/package/Root.cxx

@@ -690,14 +690,17 @@ void Root::catalogRefreshStatus(CatalogRef aCat, Delegate::StatusCode aReason)
         d->refreshing.erase(aCat);
     }
 
-    if ((aReason == Delegate::STATUS_REFRESHED) && (catIt == d->catalogs.end())) {
+    if (aCat->isUserEnabled() &&
+        (aReason == Delegate::STATUS_REFRESHED) && 
+        (catIt == d->catalogs.end())) 
+    {
         assert(!aCat->id().empty());
         d->catalogs.insert(catIt, CatalogDict::value_type(aCat->id(), aCat));
 
-        // catalog might have been previously disabled, let's remove in that case
+    // catalog might have been previously disabled, let's remove in that case
         auto j = std::find(d->disabledCatalogs.begin(),
-                           d->disabledCatalogs.end(),
-                           aCat);
+                        d->disabledCatalogs.end(),
+                        aCat);
         if (j != d->disabledCatalogs.end()) {
             SG_LOG(SG_GENERAL, SG_INFO, "re-enabling disabled catalog:" << aCat->id());
             d->disabledCatalogs.erase(j);
@@ -705,7 +708,7 @@ void Root::catalogRefreshStatus(CatalogRef aCat, Delegate::StatusCode aReason)
     }
 
     if (!aCat->isEnabled()) {
-        // catalog has errors, disable it
+        // catalog has errors or was disabled by user, disable it
         auto j = std::find(d->disabledCatalogs.begin(),
                            d->disabledCatalogs.end(),
                            aCat);