Merge pull request #3428 from ygorshenin/fixed-massive-descriptors-leaks

[index] Fixed massive descriptors leaks.

coding/file_container.cpp

@@ -3,8 +3,9 @@
 #include "coding/write_to_sink.hpp"
 #include "coding/internal/file_data.hpp"
 
+#include "std/cstring.hpp"
+
 #ifndef OMIM_OS_WINDOWS
-  #include <errno.h>
   #include <stdio.h>
   #include <unistd.h>
   #include <sys/mman.h>
@@ -18,6 +19,7 @@
   #include <windows.h>
 #endif
 
+#include <errno.h>
 
 template <class TSource, class InfoT> void Read(TSource & src, InfoT & i)
 {
@@ -122,7 +124,17 @@ void MappedFile::Open(string const & fName)
 #else
   m_fd = open(fName.c_str(), O_RDONLY | O_NONBLOCK);
   if (m_fd == -1)
-    MYTHROW(Reader::OpenException, ("Can't open file:", fName));
+  {
+    if (errno == EMFILE || errno == ENFILE)
+    {
+      MYTHROW(Reader::TooManyFilesException,
+              ("Can't open file:", fName, ", reason:", strerror(errno)));
+    }
+    else
+    {
+      MYTHROW(Reader::OpenException, ("Can't open file:", fName, ", reason:", strerror(errno)));
+    }
+  }
 #endif
 }
 

coding/reader.hpp

@@ -19,6 +19,7 @@ public:
   DECLARE_EXCEPTION(OpenException, Exception);
   DECLARE_EXCEPTION(SizeException, Exception);
   DECLARE_EXCEPTION(ReadException, Exception);
+  DECLARE_EXCEPTION(TooManyFilesException, Exception);
 
   virtual ~Reader() {}
   virtual uint64_t Size() const = 0;

indexer/index.cpp

@@ -17,9 +17,7 @@ using platform::LocalCountryFile;
 //////////////////////////////////////////////////////////////////////////////////
 
 MwmValue::MwmValue(LocalCountryFile const & localFile)
-    : m_cont(platform::GetCountryReader(localFile, MapOptions::Map)),
-      m_file(localFile),
-      m_table(0)
+  : m_cont(platform::GetCountryReader(localFile, MapOptions::Map)), m_file(localFile)
 {
   m_factory.Load(m_cont);
 }
@@ -30,14 +28,15 @@ void MwmValue::SetTable(MwmInfoEx & info)
   if (version < version::Format::v5)
     return;
 
-  if (!info.m_table)
+  m_table = info.m_table.lock();
+  if (!m_table)
   {
     if (version == version::Format::v5)
-      info.m_table = feature::FeaturesOffsetsTable::CreateIfNotExistsAndLoad(m_file, m_cont);
+      m_table = feature::FeaturesOffsetsTable::CreateIfNotExistsAndLoad(m_file, m_cont);
     else
-      info.m_table = feature::FeaturesOffsetsTable::Load(m_cont);
+      m_table = feature::FeaturesOffsetsTable::Load(m_cont);
+    info.m_table = m_table;
   }
-  m_table = info.m_table.get();
 }
 
 //////////////////////////////////////////////////////////////////////////////////
@@ -52,7 +51,7 @@ unique_ptr<MwmInfo> Index::CreateInfo(platform::LocalCountryFile const & localFi
   if (!h.IsMWMSuitable())
     return nullptr;
 
-  unique_ptr<MwmInfoEx> info(new MwmInfoEx());
+  auto info = make_unique<MwmInfoEx>();
   info->m_limitRect = h.GetBounds();
 
   pair<int, int> const scaleR = h.GetScaleRange();
@@ -85,11 +84,9 @@ bool Index::DeregisterMap(CountryFile const & countryFile) { return Deregister(c
 //////////////////////////////////////////////////////////////////////////////////
 
 Index::FeaturesLoaderGuard::FeaturesLoaderGuard(Index const & parent, MwmId const & id)
-    : m_handle(parent.GetMwmHandleById(id)),
-      /// @note This guard is suitable when mwm is loaded
-      m_vector(m_handle.GetValue<MwmValue>()->m_cont,
-               m_handle.GetValue<MwmValue>()->GetHeader(),
-               m_handle.GetValue<MwmValue>()->m_table)
+  : m_handle(parent.GetMwmHandleById(id))
+  , m_vector(m_handle.GetValue<MwmValue>()->m_cont, m_handle.GetValue<MwmValue>()->GetHeader(),
+             m_handle.GetValue<MwmValue>()->m_table.get())
 {
 }
 

indexer/index.hpp

@@ -19,12 +19,23 @@
 #include "std/limits.hpp"
 #include "std/utility.hpp"
 #include "std/vector.hpp"
-
+#include "std/weak_ptr.hpp"
 
 class MwmInfoEx : public MwmInfo
 {
-public:
-  unique_ptr<feature::FeaturesOffsetsTable> m_table;
+private:
+  friend class Index;
+  friend class MwmValue;
+
+  // weak_ptr is needed here to access offsets table in already
+  // instantiated MwmValue-s for the MWM, including MwmValues in the
+  // MwmSet's cache. We can't use shared_ptr because of offsets table
+  // must be removed as soon as the last corresponding MwmValue is
+  // destroyed. Also, note that this value must be used and modified
+  // only in MwmValue::SetTable() method, which, in turn, is called
+  // only in the MwmSet critical section, protected by a lock.  So,
+  // there's an implicit synchronization on this field.
+  weak_ptr<feature::FeaturesOffsetsTable> m_table;
 };
 
 class MwmValue : public MwmSet::MwmValueBase
@@ -33,7 +44,8 @@ public:
   FilesContainerR const m_cont;
   IndexFactory m_factory;
   platform::LocalCountryFile const m_file;
-  feature::FeaturesOffsetsTable const * m_table;
+
+  shared_ptr<feature::FeaturesOffsetsTable> m_table;
 
   explicit MwmValue(platform::LocalCountryFile const & localFile);
   void SetTable(MwmInfoEx & info);
@@ -97,7 +109,7 @@ private:
         covering::IntervalsT const & interval = cov.Get(lastScale);
 
         // Prepare features reading.
-        FeaturesVector const fv(pValue->m_cont, header, pValue->m_table);
+        FeaturesVector const fv(pValue->m_cont, header, pValue->m_table.get());
         ScaleIndex<ModelReaderPtr> index(pValue->m_cont.GetReader(INDEX_FILE_TAG),
                                          pValue->m_factory);
 
@@ -226,7 +238,8 @@ public:
       if (handle.IsAlive())
       {
         MwmValue const * pValue = handle.GetValue<MwmValue>();
-        FeaturesVector const featureReader(pValue->m_cont, pValue->GetHeader(), pValue->m_table);
+        FeaturesVector const featureReader(pValue->m_cont, pValue->GetHeader(),
+                                           pValue->m_table.get());
         do
         {
           osm::Editor::FeatureStatus const fts = editor.GetFeatureStatus(id, fidIter->m_index);
@@ -255,6 +268,7 @@ public:
   }
 
   /// Guard for loading features from particular MWM by demand.
+  /// @note This guard is suitable when mwm is loaded.
   class FeaturesLoaderGuard
   {
   public:

indexer/indexer_tests/features_vector_test.cpp

@@ -59,7 +59,7 @@ UNIT_TEST(FeaturesVectorTest_ParseMetadata)
   TEST(handle.IsAlive(), ());
 
   auto const * value = handle.GetValue<MwmValue>();
-  FeaturesVector fv(value->m_cont, value->GetHeader(), value->m_table);
+  FeaturesVector fv(value->m_cont, value->GetHeader(), value->m_table.get());
 
   map<string, int> actual;
   fv.ForEach([&](FeatureType & ft, uint32_t index)

indexer/mwm_set.cpp

@@ -1,7 +1,7 @@
 #include "indexer/mwm_set.hpp"
 #include "indexer/scales.hpp"
 
-#include "defines.hpp"
+#include "coding/reader.hpp"
 
 #include "base/assert.hpp"
 #include "base/exception.hpp"
@@ -11,6 +11,7 @@
 #include "std/algorithm.hpp"
 #include "std/sstream.hpp"
 
+#include "defines.hpp"
 
 using platform::CountryFile;
 using platform::LocalCountryFile;
@@ -162,6 +163,14 @@ bool MwmSet::DeregisterImpl(MwmId const & id, EventList & events)
     SetStatus(*info, MwmInfo::STATUS_DEREGISTERED, events);
     vector<shared_ptr<MwmInfo>> & infos = m_info[info->GetCountryName()];
     infos.erase(remove(infos.begin(), infos.end(), info), infos.end());
+    for (auto it = m_cache.begin(); it != m_cache.end(); ++it)
+    {
+      if (it->first == id)
+      {
+        m_cache.erase(it);
+        break;
+      }
+    }
     return true;
   }
 
@@ -284,6 +293,12 @@ unique_ptr<MwmSet::MwmValueBase> MwmSet::LockValueImpl(MwmId const & id, EventLi
   {
     return CreateValue(*info);
   }
+  catch (Reader::TooManyFilesException const & ex)
+  {
+    LOG(LERROR, ("Too many open files, can't open:", info->GetCountryName()));
+    --info->m_numRefs;
+    return nullptr;
+  }
   catch (exception const & ex)
   {
     LOG(LERROR, ("Can't create MWMValue for", info->GetCountryName(), "Reason", ex.what()));

indexer/mwm_set.hpp

@@ -24,8 +24,8 @@
 class MwmInfo
 {
 public:
-  friend class MwmSet;
   friend class Index;
+  friend class MwmSet;
 
   enum MwmTypeT
   {
@@ -69,7 +69,7 @@ public:
   /// Returns the lock counter value for test needs.
   uint8_t GetNumRefs() const { return m_numRefs; }
 
-private:
+protected:
   inline Status SetStatus(Status status)
   {
     Status result = m_status;
@@ -79,7 +79,7 @@ private:
 
   platform::LocalCountryFile m_file;  ///< Path to the mwm file.
   atomic<Status> m_status;            ///< Current country status.
-  uint8_t m_numRefs;                  ///< Number of active handles.
+  uint32_t m_numRefs;                 ///< Number of active handles.
 };
 
 class MwmSet
@@ -112,10 +112,7 @@ public:
   };
 
 public:
-  // Default value 32=2^5 was from the very begining.
-  // Later, we replaced my::Cache with the std::deque, but forgot to change
-  // logarithm constant 5 with actual size 32. Now it's fixed.
-  explicit MwmSet(size_t cacheSize = 32) : m_cacheSize(cacheSize) {}
+  explicit MwmSet(size_t cacheSize = 64) : m_cacheSize(cacheSize) {}
   virtual ~MwmSet() = default;
 
   class MwmValueBase

search/mwm_context.cpp

@@ -12,7 +12,7 @@ void CoverRect(m2::RectD const & rect, int scale, covering::IntervalsT & result)
 MwmContext::MwmContext(MwmSet::MwmHandle handle)
   : m_handle(move(handle))
   , m_value(*m_handle.GetValue<MwmValue>())
-  , m_vector(m_value.m_cont, m_value.GetHeader(), m_value.m_table)
+  , m_vector(m_value.m_cont, m_value.GetHeader(), m_value.m_table.get())
   , m_index(m_value.m_cont.GetReader(INDEX_FILE_TAG), m_value.m_factory)
 {
 }