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.

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

GeoTiffRasterSource Thread Safety #206

Merged
merged 3 commits into from
Jul 10, 2019

Conversation

jbouffard
Copy link
Contributor

@jbouffard jbouffard commented Jul 9, 2019

Overview

This PR makes reading from GeoTiffRasterSources thread safe by synchronizing the tiff objects within these instances. That will ensure that the state of the data doesn't change as it's being accessed by different threads.

Checklist

  • Add entry to CHANGELOG.md
  • Tests

Closes locationtech/geotrellis#2954

@jbouffard jbouffard changed the title Fix/rs threadsafety GeoTiffRasterSource Thread Safety Jul 9, 2019
Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

LGTM, though specs can be improved

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

Along with the comments I want to add, that a couple of times GeoTiffReprojectedRasterSource should be threadsafe failed with the following error:

java.lang.StackOverflowError
	at org.locationtech.proj4j.datum.AxisOrder.toENU(AxisOrder.java:122)
	at org.locationtech.proj4j.BasicCoordinateTransform.transform(BasicCoordinateTransform.java:137)
	at geotrellis.proj4.Proj4Transform$$anonfun$apply$2.apply(Transform.scala:39)
	at geotrellis.proj4.Proj4Transform$$anonfun$apply$2.apply(Transform.scala:36)
	at geotrellis.vector.reproject.Reproject$.refine$1(Reproject.scala:88)
	at geotrellis.vector.reproject.Reproject$.refine$1(Reproject.scala:97)
	... (tons of lines, it is a recursive stack)
	at geotrellis.vector.reproject.Reproject$.refine$1(Reproject.scala:97)
        ...

This actually can be related to locationtech/proj4j#29
If it is so we can workaround it by Transform.synchronized or by ReprojectRasterExtent.synchronized

It can also happen on the readBounds line targetRasterExtent.extent.reprojectAsPolygon(backTransform, 0.001) (i'm pretty sure that that is it):

val sourceExtent = closestTiffOverview.synchronized { targetRasterExtent.extent.reprojectAsPolygon(backTransform, 0.001).envelope }

…sampleRasterSource thread safe and added tests

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
@pomadchin pomadchin force-pushed the fix/rs-threadsafety branch 2 times, most recently from 8f9327f to 7426932 Compare July 10, 2019 19:32
@pomadchin pomadchin self-requested a review July 10, 2019 19:41
Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

Merging once travis is happy

@pomadchin pomadchin force-pushed the fix/rs-threadsafety branch 2 times, most recently from 4e95dc6 to fc6ed2e Compare July 10, 2019 19:48
@pomadchin pomadchin merged commit effff33 into geotrellis:master Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GeoTiffRasterSource should be thread-safe
2 participants