Opened 6 years ago

Closed 14 months ago

#855 closed defect (fixed)

Race condition in Geometry::getEnvelopeInternal

Reported by: namr Owned by: strk
Priority: major Milestone: 3.8.2
Component: Core Version: 3.6.2
Severity: Significant Keywords: Geometry::getEnvelopeInternal
Cc:

Description

When getting the intersection of two geometries, if they were not previously generated, Geos builds the Envelope for both of those. However, in a heavy multi-threaded environment, when caching the geometries, multiple threads access and generate the envelope, creating a race condition which produces a SEGFAULT. A temporary solution is to call the method getEnvelope at load time, to pre-compute it...

Attachments (1)

geos_threading.cpp (2.0 KB ) - added by steve123 4 years ago.
Test code to reproduce

Download all attachments as: .zip

Change History (10)

comment:1 by strk, 6 years ago

Thanks, GEOS (and JTS) are full of those small things, we never added any thread synchronization in the code. I'm happy to accept patches to start with it. Maybe look at c++11, the more standard solution we adopt the easier to maintain in the long run.

comment:2 by robe, 6 years ago

Milestone: 3.6.33.8.0

comment:3 by dbaston, 5 years ago

namr, can you share some test code to reproduce this?

comment:4 by pramsey, 5 years ago

Resolution: worksforme
Status: newclosed

by steve123, 4 years ago

Attachment: geos_threading.cpp added

Test code to reproduce

comment:5 by steve123, 4 years ago

Resolution: worksforme
Status: closedreopened

I have encountered the same problem in GEOS 3.8.0. I've attached some test code that segfaults about 60% of the time for me under Linux. As noted by namr, it's possible to workaround the problem by calling GEOSEnvelope_r before performing any multithreaded calculations.

comment:6 by dbaston, 4 years ago

Thank you for the test code. Technically the GEOS "thread-safe" API requires a separate context for each thread, which would resolve this problem. But it would also severely the scope of operations that can be run using multiple threads. I think it's worth addressing.

A relatively small fix is to set a mutex on whatever part of the the lazy-envelope calculation is causing the segfault.

A larger fix is to rethink the approach of lazy envelope calculation. Given that (a) geometries almost always require an envelope, and (b) envelopes can be most efficiently calculated when creating a coordinate sequence, it seems like it might make sense to eagerly calculate envelopes whenever a coordinate sequence is created. I did some experimentation along these lines a while ago; can't remember what the status of it is.

In either case, the larger fix would be something for 3.9.

comment:7 by steve123, 4 years ago

Thanks for your response. The requirements for using the thread-safe API weren't clear to me but I had concluded that it requires a different context for each thread (and I have done this in the test code). But is it also necessary to avoid sharing geometries between threads?

comment:8 by pramsey, 4 years ago

Milestone: 3.8.03.8.2

comment:9 by dbaston, 14 months ago

Resolution: fixed
Status: reopenedclosed

This should be resolved with 62edc7f05d44e83c1e70e1bf4e7f91738061d982/git

Note: See TracTickets for help on using tickets.