[GEOS] #1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[GEOS] #1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()

geos-2
#1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()
------------------------+--------------------------
 Reporter:  macdrevx    |      Owner:  geos-devel@…
     Type:  defect      |     Status:  new
 Priority:  major       |  Milestone:
Component:  Default     |    Version:  3.7.0
 Severity:  Unassigned  |   Keywords:
------------------------+--------------------------
 Original ticket: https://github.com/GEOSwift/GEOSwift/issues/192

 Actual geos version is 3.7.1 but it's not one of the available options in
 Version field.

 Two threads invoke `GEOS_init_r()` which in turn invokes
 `geos::util::Interrupt::cancel()`. Both threads write to the requested
 variable, which is detected by the [Thread
 Sanitizer|https://developer.apple.com/documentation/code_diagnostics/thread_sanitizer]
 as a data race.

--
Ticket URL: <https://trac.osgeo.org/geos/ticket/1012>
GEOS <http://trac.osgeo.org/geos>
GEOS (Geometry Engine - Open Source) is a C++ port of the Java Topology Suite (JTS).

_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GEOS] #1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()

geos-2
#1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()
------------------------+---------------------------
 Reporter:  macdrevx    |       Owner:  geos-devel@…
     Type:  defect      |      Status:  new
 Priority:  major       |   Milestone:
Component:  Default     |     Version:  3.7.0
 Severity:  Unassigned  |  Resolution:
 Keywords:              |
------------------------+---------------------------

Comment (by dbaston):

 Both threads write the same value, however. Seems like the bigger issue
 would be if one thread is checking for an interrupt while another thread
 is requesting one.

 Could change the variable in question to a std::atomic<bool>.

--
Ticket URL: <https://trac.osgeo.org/geos/ticket/1012#comment:1>
GEOS <http://trac.osgeo.org/geos>
GEOS (Geometry Engine - Open Source) is a C++ port of the Java Topology Suite (JTS).

_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GEOS] #1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()

geos-2
In reply to this post by geos-2
#1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()
------------------------+---------------------------
 Reporter:  macdrevx    |       Owner:  geos-devel@…
     Type:  defect      |      Status:  new
 Priority:  major       |   Milestone:
Component:  Default     |     Version:  3.7.0
 Severity:  Unassigned  |  Resolution:
 Keywords:              |
------------------------+---------------------------

Comment (by macdrevx):

 If I'm reading the source correctly, it seems like the interrupt mechanism
 can be used from any thread and when it is, it will interrupt only that
 one thread. Is there any reason why this shouldn't be a thread-specific or
 context-specific construct?

--
Ticket URL: <https://trac.osgeo.org/geos/ticket/1012#comment:2>
GEOS <http://trac.osgeo.org/geos>
GEOS (Geometry Engine - Open Source) is a C++ port of the Java Topology Suite (JTS).

_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GEOS] #1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()

geos-2
In reply to this post by geos-2
#1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()
------------------------+---------------------------
 Reporter:  macdrevx    |       Owner:  geos-devel@…
     Type:  defect      |      Status:  new
 Priority:  major       |   Milestone:
Component:  Default     |     Version:  master
 Severity:  Unassigned  |  Resolution:
 Keywords:              |
------------------------+---------------------------
Changes (by macdrevx):

 * cc: macdrevx (added)
 * version:  3.7.0 => master


--
Ticket URL: <https://trac.osgeo.org/geos/ticket/1012#comment:3>
GEOS <http://trac.osgeo.org/geos>
GEOS (Geometry Engine - Open Source) is a C++ port of the Java Topology Suite (JTS).

_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GEOS] #1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()

geos-2
In reply to this post by geos-2
#1012: Thread Sanitizer warns of data race in geos::util::Interrupt::cancel()
------------------------+---------------------------
 Reporter:  macdrevx    |       Owner:  geos-devel@…
     Type:  defect      |      Status:  new
 Priority:  major       |   Milestone:
Component:  Default     |     Version:  master
 Severity:  Unassigned  |  Resolution:
 Keywords:              |
------------------------+---------------------------

Comment (by macdrevx):

 I've spent some time considering what would be gained by making
 `requested` atomic. While it would avoid the data race on that particular
 memory, it would not result in a thread-safe system overall.

 Consider this example: there are long-running operations on 2 threads. A
 3rd thread requests an interrupt. The result could be either:

 A. 1 thread is interrupted (passes the check at Interrupt.cpp:66 and
 executes line 67 to lock out the other thread), or
 B. Both threads are interrupted. (Both threads pass the check at
 Interrupt.cpp:66 before either one executes line 67).

 Which scenario occurs (and within scenario A, which thread is interrupted)
 depends on the timing.

 A better solution would provide an interface that allows the consumer to
 be specific about their intent. My current thought is that for the thread
 safe CAPI we could design a new interface that would allow requesting an
 interrupt only on a specific context. Ideally, any such interface would
 avoid requiring the CAPI consumer to implement synchronization mechanisms
 around the context. Optionally, we could maintain the existing
 `GEOS_interruptRequest` interface to allow requesting interruption on
 '''all''' contexts.

 I have some implementation ideas, but I'll wait to share them until after
 getting some feedback about whether this general direction is desirable.

--
Ticket URL: <https://trac.osgeo.org/geos/ticket/1012#comment:4>
GEOS <http://trac.osgeo.org/geos>
GEOS (Geometry Engine - Open Source) is a C++ port of the Java Topology Suite (JTS).

_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel