Merge pull request #131 from ylow/master

Fix for a memory leak when creating empty sframe or sarray

Author: Jay Gu

oss_src/sframe/sarray.hpp

@@ -587,7 +587,7 @@ class sarray : public swriter_base<swriter_impl::output_iterator<T> > {
     delete writer;
     writing = false;
 
-    if (size() > 0) keep_array_file_ref();
+    keep_array_file_ref();
   }
 
   /**

oss_src/sframe/sframe.cpp

@@ -630,6 +630,12 @@ void sframe::close() {
   } else {
     index_info.nrows = 0;
   }
+
+  if (!group_index.group_index_file.empty()) {
+    index_file_handle.push_back(
+        fileio::file_handle_pool::get_instance().register_file(
+            group_index.group_index_file));
+  }
   group_writer.reset();
   write_sframe_index_file(index_file, index_info);
   inited = true;
@@ -640,7 +646,7 @@ void sframe::close() {
     columns[i]->open_for_read(group_index.columns[i]);
   }
   // we can now read.
-  if (index_info.nrows > 0) keep_array_file_ref();
+  keep_array_file_ref();
 }
 
 

oss_src/unity/lib/unity_sarray.cpp

@@ -48,6 +48,24 @@ namespace graphlab {
 
 using namespace query_eval;
 
+static std::shared_ptr<sarray<flexible_type>> get_empty_sarray() {
+  // make empty sarray and keep it around, reusing it whenever
+  // I need an empty sarray . We are intentionally leaking this object.
+  // Otherwise the termination of this will race against the cleanup of the 
+  // cache files.
+  static std::shared_ptr<sarray<flexible_type> >* empty_sarray = nullptr;
+  static graphlab::mutex static_sa_lock;
+  std::lock_guard<graphlab::mutex> guard(static_sa_lock);
+  if (empty_sarray == nullptr) {
+    empty_sarray = new std::shared_ptr<sarray<flexible_type>>();
+    (*empty_sarray) = std::make_shared<sarray<flexible_type>>();
+    (*empty_sarray)->open_for_write(1);
+    (*empty_sarray)->set_type(flex_type_enum::FLOAT);
+    (*empty_sarray)->close();
+  }
+  return *empty_sarray;
+}
+
 unity_sarray::unity_sarray() {
   // make empty sarray and keep it around, reusing it whenever
   // I need an empty sarray
@@ -288,17 +306,8 @@ void unity_sarray::save_array_by_index_file(std::string index_file) {
 }
 
 void unity_sarray::clear() {
-  static std::shared_ptr<sarray<flexible_type> > empty_sarray;
-  static graphlab::mutex static_sa_lock;
-  std::lock_guard<graphlab::mutex> guard(static_sa_lock);
-  if (empty_sarray == nullptr) {
-    empty_sarray = std::make_shared<sarray<flexible_type>>();
-    empty_sarray->open_for_write(1);
-    empty_sarray->set_type(flex_type_enum::FLOAT);
-    empty_sarray->close();
-  }
   m_planner_node = 
-      query_eval::op_sarray_source::make_planner_node(empty_sarray);
+      query_eval::op_sarray_source::make_planner_node(get_empty_sarray());
 }
 
 void unity_sarray::save(oarchive& oarc) const {

oss_src/unity/lib/unity_sframe.cpp

@@ -37,18 +37,24 @@ namespace graphlab {
 
 using namespace graphlab::query_eval;
 
-unity_sframe::unity_sframe() {
+static std::shared_ptr<sframe> get_empty_sframe() {
   // make empty sframe and keep it around, reusing it whenever
-  // I need an empty sframe
-  static std::shared_ptr<sframe> sf;
+  // I need an empty sframe. We are intentionally leaking this object.
+  // Otherwise the termination of this will race against the cleanup of the 
+  // cache files.
+  static std::shared_ptr<sframe>* sf = nullptr;
   static graphlab::mutex static_sf_lock;
   std::lock_guard<graphlab::mutex> guard(static_sf_lock);
   if (sf == nullptr) {
-    sf = std::make_shared<sframe>();
-    sf->open_for_write({}, {}, "", 1);
-    sf->close();
+    sf = new std::shared_ptr<sframe>();
+    (*sf) = std::make_shared<sframe>();
+    (*sf)->open_for_write({}, {}, "", 1);
+    (*sf)->close();
   }
-  this->set_sframe(sf);
+  return *sf;
+}
+unity_sframe::unity_sframe() {
+  this->set_sframe(get_empty_sframe());
 }
 
 unity_sframe::~unity_sframe() { clear(); }