free objects in the right context

[jw3126]

No description provided.

[jw3126]

This fixes some of the problems in #91

[jw3126]

In #91 it was actually tried to intersect objects that live in one context in another context. I am not sure if is that actually supported by the C library?

[jw3126]

LibGEOS.intersects(geo1 #=global context=#, geo2 #=global context=#, ctx #= local context =#)

[jw3126]

@nraynaud is that supported by the C-library? Was this on purpose or an oversight to use differenc contexts?

[nraynaud]

we were trying to trigger a bug on purpose, it was before confirming that even "read only" operations are unsafe.

[jw3126]

To clarify doing this operation in C code is it known to you whether that is safe?

[nraynaud]

Yes, we now know it's unsafe in C.

[jw3126]

So I think it might be a good idea to carry context around with objects (as already implemented in this PR) and also add an assertion in every function that contexts of all involved objects are equal. What do you think?

[jw3126]

Ok I will add assertions in a follow up PR to keep this one as small as possible.

[nraynaud]

agreed

…

On Mon, Oct 17, 2022 at 3:17 PM Jan Weidner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/test_geos_types.jl <#139 (comment)>: > \ No newline at end of file +end + ***@***.*** "Multi threading" begin + function f91(n) + # adapted from #91 (comment) + contexts = [LibGEOS.GEOSContext() for i=1:Threads.nthreads()] + p = [[[-1.,-1],[+1,-1],[+1,+1],[-1,+1],[-1,-1]]] + ***@***.*** :static for i=1:n + ctx = contexts[Threads.threadid()] + g1 = LibGEOS.Polygon(p, ctx) + g2 = LibGEOS.Polygon(p, ctx) So I think it might be a good idea to carry context around with objects and also add an assertion in every function that contexts of all involved objects are equal. What do you think? — Reply to this email directly, view it on GitHub <#139 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALOXOD5X3KOYDFJ4YJSQLWDVGYLANCNFSM6AAAAAARHBKNEI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jw3126]

Ok I think this PR is ready. @visr could you review?

[jw3126]

fixes #140

[visr]

This looks good to me, thanks for the contribution!

[maxfreu]

Hi @jw3126! Could you explain why there was a memory leak in the STRTree? I wrote it initally and thought it was ok. Why does the STRTree struct have to be mutable and what is GC.@preserve for?

[jw3126]

Hi @jw3126! Could you explain why there was a memory leak in the STRTree?

Sure. Generelly every create must be matched by a destroy, or else you leak memory. In your version there was a GEOSSTRtree_create_r but no GEOSSTRtree_destroy_r.

I wrote it initally and thought it was ok. Why does the STRTree struct have to be mutable

Same reason, why all other wrappers in this package are mutable. Because we need to attach finalizers and this is not possible for bitstypes.

and what is GC.@preserve for?

You are talking about this right?

    GC.@preserve context tree geometry matches begin
       GEOSSTRtree_query_r (
            context.ptr,
            tree.ptr,
            geometry.ptr,
            cfun_callback,
            pointer_from_objref(matches),
        )
    end

The thing is we only pass pointers to GEOSSTRtree_query_r. That means if no other references to these objects exist, the GC has permission to delete them before or while GEOSSTRtree_query_r is called. I don't think it will do in practice though and in this package, there are lots of similar places, where the GC has permission to delete stuff that is used via pointers.

[maxfreu]

Thank you, that sounds logical! This sounds like LibGEOS objects behave like interactive ArchGDAL objects, correct? (See https://github.com/yeesian/ArchGDAL.jl/blob/master/docs/src/memory.md)

[jw3126]

Thank you, that sounds logical! This sounds like LibGEOS objects behave like interactive ArchGDAL objects, correct? (See https://github.com/yeesian/ArchGDAL.jl/blob/master/docs/src/memory.md)

I am not familiar with ArchGDAL, but yes it seems so.