Merge pull request #102 from juanjux/fix/positions_leak

Fix leaks in HasPosition calls

Author: juanjux

bblfsh/pyuast.cc

@@ -8,22 +8,33 @@
 #include "memtracker.h"
 
 
-MemTracker memTracker;
 
 // Used to store references to the Pyobjects instanced in String() and
 // ItemAt() methods. Those can't be DECREF'ed to 0 because libuast uses them
 // so we pass ownership to these lists and free them at the end of filter()
+MemTracker memTracker;
 
+// WARNING: calls to Attribute MUST Py_DECREF the returned value once
+// used (or add it to the memtracker)
 static PyObject *Attribute(const void *node, const char *prop) {
   PyObject *n = (PyObject *)node;
   return PyObject_GetAttrString(n, prop);
 }
 
+// WARNING: calls to AttributeValue MUST Py_DECREF the returned value once
+// used (or add it to the memtracker)
 static PyObject *AttributeValue(const void *node, const char *prop) {
   PyObject *a = Attribute(node, prop);
   return a && a != Py_None ? a : NULL;
 }
 
+static bool HasAttribute(const void *node, const char *prop) {
+  PyObject *o = AttributeValue(node, prop);
+  bool res = o != NULL;
+  Py_DECREF(o);
+  return res;
+}
+
 static const char *String(const void *node, const char *prop) {
   const char *retval = NULL;
   PyObject *o = Attribute(node, prop);
@@ -163,47 +174,47 @@ static uint32_t PositionValue(const void* node, const char *prop, const char *fi
 extern "C"
 {
   static bool HasStartOffset(const void *node) {
-    return AttributeValue(node, "start_position");
+    return HasAttribute(node, "start_position");
   }
 
   static uint32_t StartOffset(const void *node) {
     return PositionValue(node, "start_position", "offset");
   }
 
   static bool HasStartLine(const void *node) {
-    return AttributeValue(node, "start_position");
+    return HasAttribute(node, "start_position");
   }
 
   static uint32_t StartLine(const void *node) {
     return PositionValue(node, "start_position", "line");
   }
 
   static bool HasStartCol(const void *node) {
-    return AttributeValue(node, "start_position");
+    return HasAttribute(node, "start_position");
   }
 
   static uint32_t StartCol(const void *node) {
     return PositionValue(node, "start_position", "col");
   }
 
   static bool HasEndOffset(const void *node) {
-    return AttributeValue(node, "end_position");
+    return HasAttribute(node, "end_position");
   }
 
   static uint32_t EndOffset(const void *node) {
     return PositionValue(node, "end_position", "offset");
   }
 
   static bool HasEndLine(const void *node) {
-    return AttributeValue(node, "end_position");
+    return HasAttribute(node, "end_position");
   }
 
   static uint32_t EndLine(const void *node) {
     return PositionValue(node, "end_position", "line");
   }
 
   static bool HasEndCol(const void *node) {
-    return AttributeValue(node, "end_position");
+    return HasAttribute(node, "end_position");
   }
 
   static uint32_t EndCol(const void *node) {

bblfsh/test.py

@@ -1,4 +1,5 @@
 import os
+import resource
 import unittest
 
 import docker
@@ -250,18 +251,45 @@ def testManyFilters(self):
         root.properties['k1'] = 'v2'
         root.properties['k2'] = 'v1'
 
-        import resource
         before = resource.getrusage(resource.RUSAGE_SELF)
         for _ in range(500):
             filter(root, "//*[@roleIdentifier]")
+
         after = resource.getrusage(resource.RUSAGE_SELF)
 
         # Check that memory usage has not doubled after running the filter
         self.assertLess(after[2] / before[2], 2.0)
 
+    def testManyParses(self):
+        before = resource.getrusage(resource.RUSAGE_SELF)
+        for _ in range(100):
+            root = self.client.parse(__file__).uast
+            root.properties['k1'] = 'v2'
+            root.properties['k2'] = 'v1'
+
+        after = resource.getrusage(resource.RUSAGE_SELF)
+
+        # Check that memory usage has not doubled after running the parse+filter
+        self.assertLess(after[2] / before[2], 2.0)
+
+    def testManyParsersAndFilters(self):
+        before = resource.getrusage(resource.RUSAGE_SELF)
+        for _ in range(100):
+            root = self.client.parse(__file__).uast
+            root.properties['k1'] = 'v2'
+            root.properties['k2'] = 'v1'
+
+            filter(root, "//*[@roleIdentifier]")
+
+        after = resource.getrusage(resource.RUSAGE_SELF)
+
+        # Check that memory usage has not doubled after running the parse+filter
+        self.assertLess(after[2] / before[2], 2.0)
+
     def _validate_filter(self, resp):
         results = filter(resp.uast, "//Import[@roleImport and @roleDeclaration]//alias")
         self.assertEqual(next(results).token, "os")
+        self.assertEqual(next(results).token, "resource")
         self.assertEqual(next(results).token, "unittest")
         self.assertEqual(next(results).token, "docker")
 

setup.py

@@ -4,7 +4,7 @@
 from setuptools import setup, find_packages, Extension
 from setuptools.command.build_ext import build_ext
 
-VERSION = "2.9.11"
+VERSION = "2.9.12"
 LIBUAST_VERSION = "v1.9.1"
 SDK_VERSION = "v1.8.0"
 SDK_MAJOR = SDK_VERSION.split('.')[0]
@@ -76,9 +76,9 @@ def getLibuast():
     if not GET_LIBUAST:
         return
 
-    runc("curl -SL https://github.com/bblfsh/libuast/releases/download/{LIBUAST_VERSION}/"
-         "libuast-{LIBUAST_VERSION}.tar.gz | tar xz")
-    runc("mv libuast-{LIBUAST_VERSION} libuast")
+    runc("curl -SL https://github.com/bblfsh/libuast/archive/{LIBUAST_VERSION}/"
+         "{LIBUAST_VERSION}.tar.gz | tar xz")
+    runc("mv {} libuast".format('libuast-' + LIBUAST_VERSION.replace('v', '')))
     runc("cp -a libuast/src bblfsh/libuast")
     runc("rm -rf libuast")