Opened 4 years ago

Last modified 20 months ago

#855 reopened defect

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 20 months ago.
Test code to reproduce

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by strk

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 Changed 3 years ago by robe

Milestone: 3.6.33.8.0

comment:3 Changed 2 years ago by dbaston

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

comment:4 Changed 2 years ago by pramsey

Resolution: worksforme
Status: newclosed

Changed 20 months ago by steve123

Attachment: geos_threading.cpp added

Test code to reproduce

comment:5 Changed 20 months ago by steve123

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 Changed 20 months ago by dbaston

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 Changed 20 months ago by steve123

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 Changed 20 months ago by pramsey

Milestone: 3.8.03.8.2
Note: See TracTickets for help on using tickets.