Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: Fix STRtree memory leaks #434

Merged
merged 3 commits into from
Nov 29, 2021
Merged

BUG: Fix STRtree memory leaks #434

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
396739d
Fix STRtree memory leaks
brendan-ward Nov 17, 2021
1940e55
xfail GEOS exceptions expected from some predicate tests for GEOS < 3.8
brendan-ward Nov 17, 2021
3b3986b
Merge remote-tracking branch 'upstream/master' into fix_strtree_memleak
jorisvandenbossche Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 21 additions & 6 deletions docker/Dockerfile.valgrind
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,31 @@
# Then run the pytest suite with valgrind enabled:
# docker run --rm pygeos/valgrind:latest valgrind --show-leak-kinds=definite --log-file=/tmp/valgrind-output python -m pytest -vv --valgrind --valgrind-log=/tmp/valgrind-output > valgrind.log

FROM python:3.6-stretch

RUN apt-get update && apt-get install -y valgrind libgeos-dev --no-install-recommends
FROM python:3.9-slim-buster

RUN pip install numpy Cython pytest pytest-valgrind
RUN apt-get update && apt-get install -y build-essential valgrind curl --no-install-recommends

COPY . /code
RUN pip install cmake numpy Cython pytest pytest-valgrind

WORKDIR /code

RUN python setup.py build_ext --inplace

ENV PYTHONMALLOC malloc

ENV LD_LIBRARY_PATH /usr/local/lib

# Build GEOS
RUN export GEOS_VERSION=3.10.1 && \
mkdir /code/geos && cd /code/geos && \
curl -OL http://download.osgeo.org/geos/geos-$GEOS_VERSION.tar.bz2 && \
tar xfj geos-$GEOS_VERSION.tar.bz2 && \
cd geos-$GEOS_VERSION && mkdir build && cd build && \
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_LIBDIR=lib .. && \
cmake --build . -j 4 && \
cmake --install . && \
cd /code

COPY . /code

# Build pygeos
RUN python setup.py build_ext --inplace && python setup.py install
72 changes: 70 additions & 2 deletions pygeos/tests/test_strtree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,70 @@ def test_query_unsupported_predicate(tree):
tree.query(pygeos.points(1, 1), predicate="disjoint")


def test_query_predicate_errors(tree):
@pytest.mark.parametrize(
"predicate,expected",
[
("intersects", [0, 1, 2]),
("within", []),
("contains", [1]),
("overlaps", []),
("crosses", []),
("covers", [0, 1, 2]),
("covered_by", []),
("contains_properly", [1]),
],
)
def test_query_prepared_inputs(tree, predicate, expected):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this test because we weren't specifically testing prepared inputs here, and wanted to verify those didn't result in memory leaks.

geom = box(0, 0, 2, 2)
pygeos.prepare(geom)
assert_array_equal(tree.query(geom, predicate=predicate), expected)


@pytest.mark.parametrize(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanded this test to verify that memory leak occurs for all predicates except intersects

"predicate",
[
pytest.param(
"intersects",
marks=pytest.mark.xfail(reason="intersects does not raise exception"),
),
pytest.param(
"within",
marks=pytest.mark.xfail(
pygeos.geos_version < (3, 8, 0), reason="GEOS < 3.8"
),
),
pytest.param(
"contains",
marks=pytest.mark.xfail(
pygeos.geos_version < (3, 8, 0), reason="GEOS < 3.8"
),
),
"overlaps",
"crosses",
"touches",
pytest.param(
"covers",
marks=pytest.mark.xfail(
pygeos.geos_version < (3, 8, 0), reason="GEOS < 3.8"
),
),
pytest.param(
"covered_by",
marks=pytest.mark.xfail(
pygeos.geos_version < (3, 8, 0), reason="GEOS < 3.8"
),
),
pytest.param(
"contains_properly",
marks=pytest.mark.xfail(
pygeos.geos_version < (3, 8, 0), reason="GEOS < 3.8"
),
),
],
)
def test_query_predicate_errors(tree, predicate):
with pytest.raises(pygeos.GEOSException):
tree.query(pygeos.linestrings([1, 1], [1, float("nan")]), predicate="touches")
tree.query(pygeos.linestrings([1, 1], [1, float("nan")]), predicate=predicate)


def test_query_tree_with_none():
Expand Down Expand Up @@ -1189,6 +1250,13 @@ def test_query_bulk_intersects_polygons(poly_tree, geometry, expected):
assert_array_equal(poly_tree.query_bulk(geometry, predicate="intersects"), expected)


def test_query_bulk_predicate_errors(tree):
with pytest.raises(pygeos.GEOSException):
tree.query_bulk(
[pygeos.linestrings([1, 1], [1, float("nan")])], predicate="touches"
)


@pytest.mark.skipif(pygeos.geos_version < (3, 6, 0), reason="GEOS < 3.6")
def test_nearest_empty_tree():
tree = pygeos.STRtree([])
Expand Down
39 changes: 26 additions & 13 deletions src/strtree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ static PyObject* STRtree_new(PyTypeObject* type, PyObject* args, PyObject* kwds)
return NULL;
}
GEOSSTRtree_query_r(ctx, tree, dummy, dummy_query_callback, NULL);
GEOSGeom_destroy_r(ctx, dummy);
}

STRtreeObject* self = (STRtreeObject*)type->tp_alloc(type, 0);
Expand Down Expand Up @@ -271,6 +272,7 @@ static char evaluate_predicate(void* context, FuncGEOS_YpY_b* predicate_func,
GEOSGeometry* geom, GEOSPreparedGeometry* prepared_geom,
tree_geom_vec_t* in_geoms, tree_geom_vec_t* out_geoms,
npy_intp* count) {
char errstate = PGERR_SUCCESS;
GeometryObject* pg_geom;
GeometryObject** pg_geom_loc; // address of geometry in tree geometries (_geoms)
GEOSGeometry* target_geom;
Expand Down Expand Up @@ -305,7 +307,9 @@ static char evaluate_predicate(void* context, FuncGEOS_YpY_b* predicate_func,
// keep the geometry if it passes the predicate
predicate_result = predicate_func(context, prepared_geom_tmp, target_geom);
if (predicate_result == 2) {
return PGERR_GEOS_EXCEPTION;
// error evaluating predicate; break and cleanup prepared geometry below
errstate = PGERR_GEOS_EXCEPTION;
break;
} else if (predicate_result == 1) {
kv_push(GeometryObject**, *out_geoms, pg_geom_loc);
(*count)++;
Expand All @@ -318,7 +322,7 @@ static char evaluate_predicate(void* context, FuncGEOS_YpY_b* predicate_func,
prepared_geom_tmp = NULL;
}

return PGERR_SUCCESS;
return errstate;
}

/* Query the tree based on input geometry and predicate function.
Expand Down Expand Up @@ -527,8 +531,6 @@ static PyObject* STRtree_query_bulk(STRtreeObject* self, PyObject* args) {

if (errstate != PGERR_SUCCESS) {
kv_destroy(query_geoms);
kv_destroy(src_indexes);
kv_destroy(target_geoms);
break;
}

Expand All @@ -543,6 +545,8 @@ static PyObject* STRtree_query_bulk(STRtreeObject* self, PyObject* args) {
GEOS_FINISH_THREADS;

if (errstate != PGERR_SUCCESS) {
kv_destroy(src_indexes);
kv_destroy(target_geoms);
return NULL;
}

Expand All @@ -554,6 +558,8 @@ static PyObject* STRtree_query_bulk(STRtreeObject* self, PyObject* args) {
result = (PyArrayObject*)PyArray_SimpleNew(2, dims, NPY_INTP);
if (result == NULL) {
PyErr_SetString(PyExc_RuntimeError, "could not allocate numpy array");
kv_destroy(src_indexes);
kv_destroy(target_geoms);
return NULL;
}

Expand Down Expand Up @@ -798,6 +804,8 @@ static PyObject* STRtree_nearest(STRtreeObject* self, PyObject* arr) {
result = (PyArrayObject*)PyArray_SimpleNew(2, index_dims, NPY_INTP);
if (result == NULL) {
PyErr_SetString(PyExc_RuntimeError, "could not allocate numpy array");
kv_destroy(src_indexes);
kv_destroy(nearest_indexes);
return NULL;
}

Expand Down Expand Up @@ -933,19 +941,13 @@ static PyObject* STRtree_nearest_all(STRtreeObject* self, PyObject* args) {
// if max_distance is defined, prescreen geometries using simple bbox expansion
if (get_bounds(ctx, geom, &xmin, &ymin, &xmax, &ymax) == 0) {
errstate = PGERR_GEOS_EXCEPTION;
kv_destroy(src_indexes);
kv_destroy(nearest_geoms);
kv_destroy(nearest_dist);
break;
}

envelope = create_box(ctx, xmin - max_distance, ymin - max_distance,
xmax + max_distance, ymax + max_distance, 1);
if (envelope == NULL) {
errstate = PGERR_GEOS_EXCEPTION;
kv_destroy(src_indexes);
kv_destroy(nearest_geoms);
kv_destroy(nearest_dist);
break;
}

Expand All @@ -961,6 +963,11 @@ static PyObject* STRtree_nearest_all(STRtreeObject* self, PyObject* args) {
}
}

if (errstate == PGERR_GEOS_EXCEPTION) {
// break outer loop
break;
}

// reset loop-dependent values of userdata
kv_init(dist_pairs);
userdata.min_distance = DBL_MAX;
Expand All @@ -972,9 +979,6 @@ static PyObject* STRtree_nearest_all(STRtreeObject* self, PyObject* args) {
if (nearest_result == NULL) {
errstate = PGERR_GEOS_EXCEPTION;
kv_destroy(dist_pairs);
kv_destroy(src_indexes);
kv_destroy(nearest_geoms);
kv_destroy(nearest_dist);
break;
}

Expand All @@ -998,6 +1002,9 @@ static PyObject* STRtree_nearest_all(STRtreeObject* self, PyObject* args) {
GEOS_FINISH_THREADS;

if (errstate != PGERR_SUCCESS) {
kv_destroy(src_indexes);
kv_destroy(nearest_geoms);
kv_destroy(nearest_dist);
return NULL;
}

Expand All @@ -1008,6 +1015,9 @@ static PyObject* STRtree_nearest_all(STRtreeObject* self, PyObject* args) {
result_indexes = (PyArrayObject*)PyArray_SimpleNew(2, index_dims, NPY_INTP);
if (result_indexes == NULL) {
PyErr_SetString(PyExc_RuntimeError, "could not allocate numpy array");
kv_destroy(src_indexes);
kv_destroy(nearest_geoms);
kv_destroy(nearest_dist);
return NULL;
}

Expand All @@ -1016,6 +1026,9 @@ static PyObject* STRtree_nearest_all(STRtreeObject* self, PyObject* args) {
result_distances = (PyArrayObject*)PyArray_SimpleNew(1, distance_dims, NPY_DOUBLE);
if (result_distances == NULL) {
PyErr_SetString(PyExc_RuntimeError, "could not allocate numpy array");
kv_destroy(src_indexes);
kv_destroy(nearest_geoms);
kv_destroy(nearest_dist);
return NULL;
}

Expand Down