Fix some memory issues (#995)

* Fix double free in test_collection
* Make sure that the memory is released when a collection goes out of scope in Python
* Removed memory leak in dlite.Collection.get_id()
* Remove commented-out lines
* Removed some debugging print statements

bindings/python/dlite-collection-python.i

@@ -192,11 +192,7 @@ class Collection(Instance):
         is registered, a DLiteError is raised.
         """
         inst = _collection_get_new(self._coll, label, metaid)
-        if inst.is_meta:
-            inst.__class__ = Metadata
-        elif inst.meta.uri == COLLECTION_ENTITY:
-            inst.__class__ = Collection
-        return inst
+        return instance_cast(inst)
 
     def get_id(self, id):
         """Return instance with given id."""

bindings/python/dlite-collection.i

@@ -53,13 +53,17 @@ const struct _Triple *
 int dlite_collection_add(struct _DLiteCollection *coll, const char *label,
                          struct _DLiteInstance *inst);
 int dlite_collection_remove(struct _DLiteCollection *coll, const char *label);
-//const struct _DLiteInstance *
-//  dlite_collection_get(const struct _DLiteCollection *coll, const char *label);
+%newobject dlite_collection_get_new;
 struct _DLiteInstance *
 dlite_collection_get_new(const struct _DLiteCollection *coll,
                          const char *label, const char *metaid);
+
+// Although dlite_collection_get_id() returns a borrowed reference in C,
+// we create a new object in Python that must be properly deallocated.
+%newobject dlite_collection_get_id;
 const struct _DLiteInstance *
   dlite_collection_get_id(const struct _DLiteCollection *coll, const char *id);
+
 int dlite_collection_has(const struct _DLiteCollection *coll,
                          const char *label);
 int dlite_collection_has_id(const struct _DLiteCollection *coll,

bindings/python/dlite-jstore.i

@@ -102,6 +102,7 @@ struct _JStore {};
   }
 
   %feature("docstring", "Iterate over all id's matching pattern.") get_ids;
+  %newobject get_ids;
   struct _DLiteJStoreIter *get_ids(const char *pattern=NULL) {
     return dlite_jstore_iter_create($self, pattern);
   }

bindings/python/dlite-misc-python.i

@@ -81,8 +81,8 @@ class errctl():
         return [_dlite._err_getcode(errname) for errname in errnames]
 
 
-silent = errctl(filename="None")
-"""Context manager for a silent code block.  Same as `errctl(filename="None")`."""
+silent = errctl(hide=True)
+"""Context manager for a silent code block.  Same as `errctl(hide=True)`."""
 
 # A set for keeping track of already issued deprecation warnings
 _deprecation_warning_record = set()

bindings/python/tests/test_collection.py

@@ -99,7 +99,6 @@
 coll.add("inst1", inst1, force=True)  # revert
 assert coll.get("inst1") == inst1
 
-
 # Test convinience functions
 i1 = coll.get_id(inst1.uuid)
 assert i1 == inst1

bindings/python/tests/test_mapping.py

@@ -16,7 +16,6 @@
 person.name = "Neil Armstrong"
 person.age = 39
 person.skills = ["keping the head cold", "famous quotes"]
-# person.incref()
 
 # Map person to an instance of SimplePerson
 simple = dlite.mapping("http://onto-ns.com/meta/0.1/SimplePerson", [person])

cmake/valgrind-dlite.supp

@@ -35,9 +35,22 @@
    fun:calloc
    fun:register_plugin
    fun:plugin_load
-   fun:plugin_get_api
-   fun:dlite_storage_plugin_get
-   fun:dlite_storage_open
+}
+
+
+# ------------------------------------
+# rdflib
+# ------------------------------------
+
+{
+   rdflib-model
+   Memcheck:Leak
+   match-leak-kinds: indirect
+   fun:calloc
+   ...
+   fun:librdf_new_storage_from_factory
+   fun:triplestore_create_with_world
+   fun:triplestore_create
 }
 
 

doc/contributors_guide/tips_and_tricks.md

@@ -172,6 +172,11 @@ to rerun valgrind on that issue, you can run
 
 Both of the above commands will write the output to `<BUILD_DIR>/Testing/Temporary/MemoryChecker.<#>.log`, where `<#>` is the test number.
 
+```note
+The first time you run `make memcheck`, a supression file will be created, suppressing issues that does not comes from DLite.
+Generating a suppression file may take long time, but it will only be generated once unless you manually add more suppressions to one of the `cmake/valgrind-*.supp` files.
+```
+
 ---
 
 Alternatively, you can run valgrind manually, with

src/dlite-collection.c

@@ -45,7 +45,8 @@ int dlite_collection_deinit(DLiteInstance *inst)
   while ((r=dlite_collection_find(coll,&state, NULL, "_has-uuid", NULL,
                                   NULL))) {
     if ((inst2 = dlite_instance_get(r->o))) {
-      dlite_instance_decref(inst2);
+      dlite_instance_decref(inst2);  // remove ref from collection
+      dlite_instance_decref(inst2);  // remove local ref to inst2
     } else {
       warnx("cannot remove missing instance: %s", r->o);
     }
@@ -601,10 +602,12 @@ DLiteInstance *dlite_collection_get_new(const DLiteCollection *coll,
   if (!inst) return NULL;
   if (metaid) {
     if (!(inst = dlite_mapping(metaid, (const DLiteInstance **)&inst, 1)))
-      errx(dliteMappingError,
-           "cannot map instance labeled '%s' to '%s'", label, metaid);
-  } else
+      return errx(dliteMappingError,
+                  "cannot map instance labeled '%s' to '%s'",
+                  label, metaid), NULL;
+  } else {
     dlite_instance_incref(inst);
+  }
   return inst;
 }
 

src/dlite-mapping.c

@@ -291,6 +291,7 @@ DLiteInstance *mapping_map_rec(const DLiteMapping *m, Instances *instances)
 
   /* Add new instance to `instances` */
   assert(strcmp(inst->meta->uri, m->output_uri) == 0);
+  dlite_instance_incref(inst);  // TODO: is this correct?
   map_set(instances, inst->meta->uri, inst);
 
  fail:
@@ -347,8 +348,8 @@ int set_inputs(Instances *inputs, const DLiteInstance **instances, int n)
     const char *uri = instances[i]->meta->uri;
     if (map_get(inputs, uri)) {
       while (--i >= 0) {
-        dlite_instance_decref((DLiteInstance *)instances[i]);
         map_remove(inputs, instances[i]->meta->uri);
+        dlite_instance_decref((DLiteInstance *)instances[i]);
       }
       return err(1, "more than one instance of the same metadata: %s", uri);
     }
@@ -358,6 +359,20 @@ int set_inputs(Instances *inputs, const DLiteInstance **instances, int n)
   return 0;
 }
 
+/* Decrease the refcount on each instance in inputs. */
+int decref_inputs(Instances *inputs)
+{
+  const char *uri;
+  map_iter_t iter = map_iter(inputs);
+  while ((uri = map_next(inputs, &iter))) {
+    DLiteInstance **instp = map_get(inputs, uri);
+    if (instp) dlite_instance_decref(*instp);
+  }
+  return 0;
+}
+
+
+
 /* Recursive help function that removes all references to input instances
    found in `m`, from `inputs`. */
 void remove_inputs_rec(const DLiteMapping *m, Instances *inputs)
@@ -430,7 +445,6 @@ DLiteInstance *dlite_mapping_map(const DLiteMapping *m,
 DLiteInstance *dlite_mapping(const char *output_uri,
                              const DLiteInstance **instances, int n)
 {
-  int i;
   DLiteInstance *inst=NULL;
   DLiteMapping *m=NULL;
   Instances inputs;
@@ -443,9 +457,11 @@ DLiteInstance *dlite_mapping(const char *output_uri,
   inst = dlite_mapping_map(m, instances, n);
 
  fail:
-  map_deinit(&inputs);
   if (m) dlite_mapping_free(m);
+
   /* Decrease refcount */
-  for (i=0; i<n; i++) dlite_instance_decref((DLiteInstance *)instances[i]);
+  decref_inputs(&inputs);
+  map_deinit(&inputs);
+
   return inst;
 }

src/tests/python/test_python_mapping.c

@@ -58,6 +58,7 @@ MU_TEST(test_map)
   dlite_instance_save_url("json://inst3.json", inst3);
 
   dlite_instance_decref(inst3);
+  dlite_instance_decref(ent3);
   //dlite_instance_decref((DLiteInstance *)instances[0]);
 }
 

src/tests/test_collection.c

@@ -238,32 +238,12 @@ MU_TEST(test_collection_load)
 {
   DLiteCollection *coll2;
   char *collpath = STRINGIFY(dlite_SOURCE_DIR) "/src/tests/coll.json";
-  //DLiteStoragePathIter *iter;
-  //const char *path;
-
-  dlite_storage_paths_append(STRINGIFY(dlite_SOURCE_DIR) "/src/tests/*.json");
-
-  //printf("\n\nStorage paths:\n");
-  //iter = dlite_storage_paths_iter_start();
-  //while ((path = dlite_storage_paths_iter_next(iter)))
-  //  printf("  - %s\n", path);
-  //printf("\n");
-  //dlite_storage_paths_iter_stop(iter);
-
   FILE *fp = fopen(collpath, "r");
+  dlite_storage_paths_append(STRINGIFY(dlite_SOURCE_DIR) "/src/tests/*.json");
   coll2 = (DLiteCollection *)
     dlite_json_fscan(fp, NULL, "http://onto-ns.com/meta/0.1/Collection");
   fclose(fp);
-  //printf("\n\n--- coll2: %p ---\n", (void *)coll2);
-  //dlite_json_print((DLiteInstance *)coll2);
-  //printf("----------------------\n");
-
-  const DLiteInstance *inst = dlite_collection_get(coll2, "inst");
-  //printf("\n--- inst: %p ---\n", (void *)inst);
-  //dlite_json_print((DLiteInstance *)inst);
-  //printf("----------------------\n");
 
-  dlite_instance_decref((DLiteInstance *)inst);
   dlite_collection_decref(coll2);
 }
 

src/tests/test_json.c

@@ -152,9 +152,10 @@ MU_TEST(test_append)
 
 MU_TEST(test_decref)
 {
-  dlite_instance_decref(coll);
-  dlite_instance_decref(inst);
-  dlite_meta_decref(meta);
+  int n;
+  for (n=coll->_refcount; n>0; n--) dlite_instance_decref(coll);
+  for (n=inst->_refcount; n>0; n--) dlite_instance_decref(inst);
+  for (n=meta->_refcount; n>1; n--) dlite_meta_decref(meta);  // metadata
   coll = NULL;
   inst = NULL;
   meta = NULL;

src/triplestore-redland.c

@@ -466,8 +466,8 @@ TripleStore *triplestore_create()
 void triplestore_free(TripleStore *ts)
 {
   Globals *g = get_globals();
- assert(g->nmodels > 0);
- g->nmodels--;
+  assert(g->nmodels > 0);
+  g->nmodels--;
   librdf_free_storage(ts->storage);
   librdf_free_model(ts->model);
   if (ts->storage_name)  free((char *)ts->storage_name);