Fieldlocation shared ptr issues

src/fdb5/api/RemoteFDB.cc

@@ -14,6 +14,7 @@
  */
 
 #include <functional>
+#include <memory>
 #include <unistd.h>
 
 #include "fdb5/api/RemoteFDB.h"
@@ -555,7 +556,7 @@ struct InspectHelper : BaseAPIHelper<ListElement, fdb5::remote::Message::Inspect
 
     static ListElement valueFromStream(eckit::Stream& s, RemoteFDB* fdb) {
         ListElement elem(s);
-        return ListElement(elem.key(), RemoteFieldLocation(fdb, elem.location()).make_shared(), elem.timestamp());
+        return {elem.key(), std::make_shared<RemoteFieldLocation>(fdb, elem.sharedLocation()), elem.timestamp()};
     }
 };
 

src/fdb5/api/helpers/ListIterator.h

@@ -49,6 +49,9 @@ class ListElement {
     ListElement(eckit::Stream& s);
 
     const std::vector<Key>& key() const { return keyParts_; }
+
+    std::shared_ptr<const FieldLocation> sharedLocation() { return location_; }
+
     const FieldLocation& location() const { return *location_; }
     const time_t& timestamp() const { return timestamp_; }
 

src/fdb5/database/Field.cc

@@ -10,23 +10,18 @@
 
 #include "fdb5/database/Field.h"
 
+#include <memory>
+#include <ostream>
+#include <utility>
+
 namespace fdb5 {
 
 //----------------------------------------------------------------------------------------------------------------------
 
 Field::Field() {}
 
-Field::Field(std::shared_ptr<FieldLocation> location, time_t timestamp, const FieldDetails& details):
-    location_(std::move(location)),
-    timestamp_(timestamp),
-    details_(details) {
-}
-
-Field::Field(const FieldLocation&& location, time_t timestamp, const FieldDetails& details):
-    location_(location.make_shared()),
-    timestamp_(timestamp),
-    details_(details) {
-}
+Field::Field(std::shared_ptr<FieldLocation> location, time_t timestamp, FieldDetails details):
+    location_(std::move(location)), timestamp_(timestamp), details_(std::move(details)) { }
 
 void Field::print(std::ostream& out) const {
     out << "Field(location=" << location_;

src/fdb5/database/Field.h

@@ -43,8 +43,7 @@ class Field {
 
     Field();
 
-    Field(std::shared_ptr<FieldLocation> location, time_t timestamp, const FieldDetails& details = FieldDetails());
-    Field(const FieldLocation&& location, time_t timestamp, const FieldDetails& details = FieldDetails());
+    Field(std::shared_ptr<FieldLocation> location, time_t timestamp, FieldDetails details = FieldDetails());
 
     eckit::DataHandle* dataHandle() const { return location_->dataHandle(); }
 
@@ -63,7 +62,7 @@ class Field {
 
    std::shared_ptr<FieldLocation> location_;
 
-    time_t timestamp_;
+   time_t timestamp_ {0};
 
     FieldDetails details_;
 

src/fdb5/database/FieldLocation.h

@@ -17,6 +17,7 @@
 #define fdb5_FieldLocation_H
 
 #include <memory>
+
 #include <eckit/filesystem/URI.h>
 
 #include "eckit/filesystem/PathName.h"
@@ -36,8 +37,7 @@ namespace fdb5 {
 
 class FieldLocationVisitor;
 
-class FieldLocation : public eckit::OwnedLock, public eckit::Streamable {
-
+class FieldLocation: public eckit::OwnedLock, public eckit::Streamable, public std::enable_shared_from_this<FieldLocation> {
 public: // methods
 
     FieldLocation() : offset_(eckit::Offset(0)), length_(eckit::Length(0)), remapKey_(Key()) {}
@@ -57,10 +57,13 @@ class FieldLocation : public eckit::OwnedLock, public eckit::Streamable {
 
     virtual eckit::DataHandle *dataHandle() const = 0;
 
-    /// Create a (shared) copy of the current object, for storage in a general container.
-    virtual std::shared_ptr<FieldLocation> make_shared() const = 0;
+    std::shared_ptr<FieldLocation> sharedPtr() { return shared_from_this(); }
+
+    std::shared_ptr<const FieldLocation> sharedPtr() const { return shared_from_this(); }
+
+    virtual std::shared_ptr<FieldLocation> stableLocation() { return shared_from_this(); }
 
-    virtual std::shared_ptr<FieldLocation> stableLocation() const { return make_shared(); }
+    virtual std::shared_ptr<const FieldLocation> stableLocation() const { return shared_from_this(); }
 
     virtual void visit(FieldLocationVisitor& visitor) const = 0;
 

src/fdb5/remote/RemoteFieldLocation.cc

@@ -19,6 +19,8 @@
 #include "eckit/exception/Exceptions.h"
 #include "eckit/filesystem/URIManager.h"
 
+#include <utility>
+
 namespace fdb5 {
 namespace remote {
 
@@ -27,12 +29,13 @@ ::eckit::Reanimator<RemoteFieldLocation> RemoteFieldLocation::reanimator_;
 
 //----------------------------------------------------------------------------------------------------------------------
 
-RemoteFieldLocation::RemoteFieldLocation(RemoteFDB* remoteFDB, const FieldLocation& remoteLocation) :
-    FieldLocation(eckit::URI("fdb", remoteFDB->controlEndpoint().host(),  remoteFDB->controlEndpoint().port())),
+RemoteFieldLocation::RemoteFieldLocation(RemoteFDB* remoteFDB, std::shared_ptr<const FieldLocation> remoteLocation):
+    FieldLocation(eckit::URI("fdb", remoteFDB->controlEndpoint().host(), remoteFDB->controlEndpoint().port())),
     remoteFDB_(remoteFDB),
-    internal_(remoteLocation.make_shared()) {
+    internal_(std::move(remoteLocation)) {
     ASSERT(remoteFDB);
-    ASSERT(remoteLocation.uri().scheme() != "fdb");
+    ASSERT(remoteLocation);
+    ASSERT(remoteLocation->uri().scheme() != "fdb");
 }
 
 RemoteFieldLocation::RemoteFieldLocation(const eckit::URI& uri) :
@@ -52,11 +55,6 @@ RemoteFieldLocation::RemoteFieldLocation(const RemoteFieldLocation& rhs) :
     remoteFDB_(rhs.remoteFDB_),
     internal_(rhs.internal_) {}
 
-
-std::shared_ptr<FieldLocation> RemoteFieldLocation::make_shared() const {
-    return std::make_shared<RemoteFieldLocation>(std::move(*this));
-}
-
 eckit::DataHandle* RemoteFieldLocation::dataHandle() const {
     ASSERT(remoteFDB_);
     return remoteFDB_->dataHandle(*internal_);
@@ -118,4 +116,4 @@ class FdbURIManager : public eckit::URIManager {
 static FdbURIManager manager_fdb_file("fdb");
 
 } // namespace remote
-} // namespace fdb5
+} // namespace fdb5

src/fdb5/remote/RemoteFieldLocation.h

@@ -21,6 +21,8 @@
 
 #include "fdb5/database/FieldLocation.h"
 
+#include <memory>
+
 namespace fdb5 {
 
 class RemoteFDB;
@@ -31,8 +33,7 @@ namespace remote {
 
 class RemoteFieldLocation : public FieldLocation {
 public:
-
-    RemoteFieldLocation(RemoteFDB* remoteFDB, const FieldLocation& remoteLocation);
+    RemoteFieldLocation(RemoteFDB* remoteFDB, std::shared_ptr<const FieldLocation> remoteLocation);
     RemoteFieldLocation(const eckit::URI &uri);
     RemoteFieldLocation(const eckit::URI &uri, const eckit::Offset &offset, const eckit::Length &length, const Key& remapKey);
     RemoteFieldLocation(eckit::Stream&);
@@ -43,7 +44,6 @@ class RemoteFieldLocation : public FieldLocation {
 
     virtual eckit::DataHandle *dataHandle() const override;
 
-    virtual std::shared_ptr<FieldLocation> make_shared() const override;
     virtual void visit(FieldLocationVisitor& visitor) const override;
 
 public: // For Streamable

src/fdb5/toc/TocCatalogueWriter.cc

@@ -22,6 +22,7 @@
 #include "fdb5/toc/TocFieldLocation.h"
 #include "fdb5/toc/TocIndex.h"
 #include "fdb5/io/LustreSettings.h"
+#include <memory>
 
 using namespace eckit;
 
@@ -129,7 +130,7 @@ void TocCatalogueWriter::index(const Key &key, const eckit::URI &uri, eckit::Off
         selectIndex(currentIndexKey_);
     }
 
-    Field field(TocFieldLocation(uri, offset, length, Key()), currentIndex().timestamp());
+    Field field(std::make_shared<TocFieldLocation>(uri, offset, length, Key()), currentIndex().timestamp());
 
     current_.put(key, field);
 

src/fdb5/toc/TocFieldLocation.cc

@@ -43,10 +43,6 @@ TocFieldLocation::TocFieldLocation(const UriStore &store, const FieldRef &ref) :
 TocFieldLocation::TocFieldLocation(eckit::Stream& s) :
     FieldLocation(s) {}
 
-std::shared_ptr<FieldLocation> TocFieldLocation::make_shared() const {
-    return std::make_shared<TocFieldLocation>(std::move(*this));
-}
-
 eckit::DataHandle *TocFieldLocation::dataHandle() const {
     if (remapKey_.empty()) {
         return uri_.path().partHandle(offset(), length());

src/fdb5/toc/TocFieldLocation.h

@@ -39,8 +39,6 @@ class TocFieldLocation : public FieldLocation {
 
     eckit::DataHandle* dataHandle() const override;
 
-    virtual std::shared_ptr<FieldLocation> make_shared() const override;
-
     virtual void visit(FieldLocationVisitor& visitor) const override;
 
 public: // For Streamable

src/fdb5/toc/TocIndex.cc

@@ -8,14 +8,15 @@
  * does it submit to any jurisdiction.
  */
 
-#include "eckit/log/BigNum.h"
-
 #include "fdb5/LibFdb5.h"
-#include "fdb5/toc/TocStats.h"
-#include "fdb5/toc/TocIndex.h"
+#include "fdb5/database/FieldLocation.h"
 #include "fdb5/toc/BTreeIndex.h"
 #include "fdb5/toc/FieldRef.h"
 #include "fdb5/toc/TocFieldLocation.h"
+#include "fdb5/toc/TocIndex.h"
+#include "fdb5/toc/TocStats.h"
+
+#include <memory>
 
 namespace fdb5 {
 
@@ -85,9 +86,9 @@ bool TocIndex::get(const Key &key, const Key &remapKey, Field &field) const {
     bool found = btree_->get(key.valuesToString(), ref);
     if ( found ) {
         const eckit::URI& uri = files_.get(ref.uriId());
-        FieldLocation* loc = FieldLocationFactory::instance().build(uri.scheme(), uri, ref.offset(), ref.length(), remapKey);
-        field = Field(std::move(*loc), timestamp_, ref.details());
-        delete(loc);
+        std::shared_ptr<FieldLocation> location(
+            FieldLocationFactory::instance().build(uri.scheme(), uri, ref.offset(), ref.length(), remapKey));
+        field = Field(location, timestamp_, ref.details());
     }
     return found;
 }
@@ -173,7 +174,7 @@ class TocIndexVisitor : public BTreeIndexVisitor {
         visitor_(visitor) {}
 
     void visit(const std::string& keyFingerprint, const FieldRef& ref) {
-        Field field(TocFieldLocation(files_, ref), visitor_.indexTimestamp(), ref.details());
+        Field field(std::make_shared<TocFieldLocation>(files_, ref), visitor_.indexTimestamp(), ref.details());
         visitor_.visitDatum(field, keyFingerprint);
     }
 };

tests/fdb/database/CMakeLists.txt

@@ -7,3 +7,8 @@ ecbuild_add_test( TARGET test_fdb5_database_indexaxis
     INCLUDES ${PMEM_INCLUDE_DIRS}
     LIBS fdb5
     ENVIRONMENT "${_test_environment}")
+
+ecbuild_add_test( TARGET test_fdb5_fieldlocation
+    SOURCES test_fieldlocation.cc
+    LIBS fdb5
+    ENVIRONMENT "${_test_environment}")

tests/fdb/database/test_fieldlocation.cc

@@ -0,0 +1,65 @@
+/*
+ * (C) Copyright 1996- ECMWF.
+ *
+ * This software is licensed under the terms of the Apache Licence Version 2.0
+ * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0.
+ * In applying this licence, ECMWF does not waive the privileges and immunities
+ * granted to it by virtue of its status as an intergovernmental organisation nor
+ * does it submit to any jurisdiction.
+ */
+
+#include "fdb5/database/FieldLocation.h"
+
+#include "eckit/filesystem/URI.h"
+#include "eckit/testing/Test.h"
+
+#include <memory>
+
+using namespace eckit;
+using namespace eckit::testing;
+
+namespace fdb::test {
+
+//----------------------------------------------------------------------------------------------------------------------
+
+CASE("FieldLocation - shared_ptr") {
+    URI uri {"dummy_uri"};
+
+    // GOOD (not best)
+    {
+        auto location = std::shared_ptr<fdb5::FieldLocation>(fdb5::FieldLocationFactory::instance().build("file", uri));
+
+        auto loc1 = location->sharedPtr();
+        EXPECT_EQUAL(loc1.use_count(), 2);
+
+        auto loc2 = location->sharedPtr();
+        EXPECT_EQUAL(loc2.use_count(), 3);
+
+        // check that the shared pointers are the same
+        EXPECT_EQUAL(loc2, loc1);
+
+        {
+            auto loc3 = location->sharedPtr();
+            EXPECT_EQUAL(loc3.use_count(), 4);
+            EXPECT_EQUAL(loc3, loc1);
+        }
+
+        auto loc4 = location->sharedPtr();
+        EXPECT_EQUAL(loc4.use_count(), 4);
+        EXPECT_EQUAL(loc4, loc1);
+    }
+
+    // BAD: this is a how NOT to use shared_ptr on FieldLocation
+    {
+        auto location = std::unique_ptr<fdb5::FieldLocation>(fdb5::FieldLocationFactory::instance().build("file", uri));
+        EXPECT_THROWS_AS(location->sharedPtr(), std::bad_weak_ptr);
+    }
+}
+
+//----------------------------------------------------------------------------------------------------------------------
+
+}  // namespace fdb::test
+
+int main(int argc, char** argv) {
+    return run_tests(argc, argv);
+}