georust / geos

Rust bindings for GEOS
https://docs.rs/geos/
MIT License
122 stars 41 forks source link

Thread safety of geos context and lifetimes #141

Open vladaburian opened 9 months ago

vladaburian commented 9 months ago

Hi,

I have few questions/suggestions about geos context and lifetimes. (I'm literally couple of days into learning rust so I may not see some obvious things.)

1) GEOS context

I don't understand why each geometry keeps geos context and why is this context cloned over to other derived geometries (even though the cloned rust context is referencing the same instance of geos context).

Geos context is ment to be used as a thread safe mechanism to propagate C++ exceptions through C api and to keep some temporary/helper data AFAIK (like buffer to format error messages or geos Point instance to be re-used as a temporary object). But rust binding does allow non-thread safe usage.

I think it would be good to have thread local instances of geos context and to use those temporarily just to call C geos api. This wayite would also prevent unnecessary allocations of new contexts in many operations (like creating new non-derived geometries, new coordinate sequences, etc.). In the end the whole context mechanism would be private (and transparent to the users of rust api).

The only thing in geos context which is not primarily related to thread safety is configuration of WKB dims and byte order. But I feel like it does not belong there and also it has been deprecated.

There is draft of what I mean: https://github.com/georust/geos/commit/8c096818dd8b73dfdee93ccad77dcec743c48e0f

links:

2) Lifetimes

I don't understand why Geometry struct (and other structs) has explicit lifetime and why all functions returning new derived geometries also have the same lifetime. Because derived geometries resulting from geometric operations, etc. are just new and owned geometries non-related to the input geometry/geometries.

(It's true that derived geos geometries do pass around non-owned pointer to geometry factory, which is also part of geos context, but which cannot be changed with C api and which is always set to the single global instance.)

It seems to me that all lifetime parameters could be removed and the code would still be correct. And then it would be possible to pass Geometry freely around without any constraints. (But as I wrote, I do not have much experience with rust yet. Maybe I'm missing something.)

There is draft of what I mean: https://github.com/georust/geos/commit/83c32190fd253fb3459f2746d89b1906e671d79c

GuillaumeGomez commented 9 months ago

All the problems you describe come from my (very) limited understanding of GEOS.

Basically it all comes down to this: is it useful to be able to provide your own context to a type? With the suggested changes, we can't set an error handler to a specific type or even a specific context to a type since they all share the same one.