Reconsidering PJ* holding a PJ_CONTEXT ?

[rouault]

Was actually raised in #1047 . I've hit that issue several time. Last time just a few minutes ago : OSGeo/gdal#2047

The PJ* object holds a context mostly for the purpose of setting errno or calling the logging callbacks. However this strategy of binding the context in the PJ makes it inconvenient to use in multi-threaded code where the context is a per-thread object. If you create a PJ* object in one thread T1, then transfer it to thread T2, that thread T2 terminates (and thus its associated context is destroyed if you bind its lifetime to a TLS context), then the PJ* points to a destroyed context.

An idea, if we want to maintain backward compat, could be to add a proj_destroy_with_context() and proj_trans_with_context(), that would reassign the context, and strongly deprecate proj_destroy() and proj_trans(). PJ* would still hold a context, but it would be overriden by later calls.

Just thinking out loud

[snowman2]

For pyproj in order to keep things safe, each PJ* has its own PJ_CONTEXT* and does not share it at all (not efficient to have to re-create a context for each PJ*). That way, they are always deallocated together:
https://github.com/pyproj4/pyproj/blob/1350dd576edfa96666665020d900e68985ecf1f2/pyproj/_crs.pyx#L325-L330

This is why I raised the issue about the proj_assign_context() #591 a while back.

[rouault]

has its own PJ_CONTEXT* and does not share it at all (not efficient to have to re-create a context for each PJ*

yes, but performance will suck in a number of use cases. That's not the cost of creating the context each time by itsel, but rather the fact that you can benefit from the caches of PJ_CONTEXT. This is at least true in situations where you may have to instantiate different transformations

[kbevers]

and strongly deprecate proj_destroy() and proj_trans().

but if

PJ* would still hold a context, but it would be overriden by later calls.

is true, is it then really necessary to deprecate proj_trans and proj_destryoy(). Is there a way to do this without breaking the API?

[rouault]

is it then really necessary to deprecate proj_trans and proj_destryoy()

By "deprecate" I meant discourage their use, but keep them. That could be done in a light way with a big warning in the documentation of the functions. Another possibility is to use the __attribute__ ((deprecated)) annotation supported by gcc & clang in a function declaration, so that people notice it when they build their code (possibly conditionalized with a #ifdef so that people who build with -Werror have a way to use the deprecated functions)

[kbevers]

By "deprecate" I meant discourage their use, but keep them.

Got it. I'm on board with that.

[snowman2]

Sounds like a good plan 👍

[rouault]

Assigning milestone 7.0 at least so that we remember to consider it, or defer

[benstadin]

I find the way SQLite handles threading via configuration a good design. SQLite allows to choose the threading mode at runtime [1]. For example when setting proj to multitthreaded mode the application could responsible to handle access to the context. Otherwise proj should synchronize access to the context - by default.

It avoids to make assumptions about how an application handles multi threading. For example I have worker threads handling web services in my application and want to reuse proj contexts (consider I have 4 threads and a pool of proj contexts in my kind of application). In such scenario I can serialize the creation and destruction of proj contexts, and can assure that a proj context will be used by one worker only at a given time (fetched from a pool and returned when done).

This example might not be typical. But using SQLite for many years in different environments I find the design very solid. It can work out of the box in multithreaded environmemnts using locks on its own per default while providing configuration options to handle threading on the application side when desired.

[1] https://www.sqlite.org/c3ref/c_config_covering_index_scan.html#sqliteconfigmultithread

[kbevers]

Assigning milestone 7.0 at least so that we remember to consider it, or defer

@rouault it is probably about time to consider this again: Is this something you want to do or should we remove the issue from the 7.0 milestone?

[rouault]

not sure I'll have time. we can keep the 7.0 milestone for now and postpone it if it hasn't been dealt with

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.