Changes to render thread-safe the package, with test.

[fpl]

This patch closes #58 . Even removed the string_pointer use which is obsolete since FFI::Platypus 0.61. Added a t/threads.t test to verify that multi-threading does not crash.

Basically, CPLPushErrorHandler() must be performed per thread, not at new() time for the whole program, that is after creation of any thread. That implies that there are two possibilities:

• avoid the singleton pattern, so perform the Geo::GDAL::FFI initialization for any thread after its creation.
• separate error handling initialization from general initialization.

This patch follows the second approach. Note the use of sticky mode for closure() as required in the documentation.
Not sure the DESTROY block makes sense, but it seems it does not hurt.

[shawnlaffan]

Works for me on with Strawberry Perl 5.38, and unthreaded perl 5.36 on Ubuntu.

t/open.t is now noisier although the tests pass.

t/open.t ............... ERROR 4: `test.shp' not recognized as a supported file format.
t/open.t ............... ok

[shawnlaffan]

It seems the VERBOSE_ERROR output in that test is going straight to stderr.

[fpl]

It seems the VERBOSE_ERROR output in that test is going straight to stderr.

That's because now

 Geo::GDAL::FFI->get_instance->SetErrorHandling;

should be used before any other thing. That is the side effect of removing it in the new() sub for thread safety.
A proper way to manage this kind of thing is hiding all those details in a new one-line init() sub to be used just before any other method as

Geo::GDAL::FFI->init();

That could be safely called in single/main threads or even in any other threads. In practice, a little change in the API.
Sorry, but I cannot see any other way to deal with this sort of thing.

Of course if this change is acceptable, I can append it to this PR, and properly change documentation (and propose a version change to track the API modification).

[shawnlaffan]

Having an explicit init call might be a reasonable thing to do.

Alternately the new method could be refactored to not do any of the function bindings, instead only setting the error handler and any other thread specific properties that come up. I think it would still need a singleton pattern, though, as there should be only one error handler per thread.

Any of the above changes would be separate to this PR.

@ajolma - what do you think?

[ajolma]

I haven't followed this much so I don't have a very educated opinion.

One this is that those changes to the attach method calls should be done to build-tools/parse_h.pl, which is used to create them by running against GDAL sources.

[shawnlaffan]

parse_h.pl is changed as part of the first commit.

[fpl]

One this is that those changes to the attach method calls should be done to build-tools/parse_h.pl, which is used to create them by running against GDAL sources.

Talking about that, even issue #53, which is managed by PR #68, would require an extensive change to the same build tool, as explained in the same issue.

[fpl]

Having an explicit init call might be a reasonable thing to do.

Alternately the new method could be refactored to not do any of the function bindings, instead only setting the error handler and any other thread specific properties that come up. I think it would still need a singleton pattern, though, as there should be only one error handler per thread.

Any of the above changes would be separate to this PR.

@ajolma - what do you think?

IMHO, the new() method does the right thing as a singleton. The only setup that needs to be moved out of the BEGIN block is the GDAL error handling that needs to be performed per thread instead. As a general pattern, any lib binding should have a double-step initialization, one for the general setup and one per thread, to ensure thread safety. Other packages do not have a general setup and require multiple instances (one for each thread) to work. In this case, I think it's a bit too heavy loading all the FFI stuff for every thread by removing the singleton constraint at all, if that is what you suggest.

[shawnlaffan]

IMHO, the new() method does the right thing as a singleton.

The issue is that this would be a breaking change for existing code.

The FFI attach calls only need to be done once when the module is loaded, so they can be refactored out of the new sub. The new sub could then be called per-thread and set the error handling as it does now. This means it is essentially the init call.

Unless of course there are complications due to threads...

[fpl]

IMHO, the new() method does the right thing as a singleton.

The issue is that this would be a breaking change for existing code.

The FFI attach calls only need to be done once when the module is loaded, so they can be refactored out of the new sub. The new sub could then be called per-thread and set the error handling as it does now. This means it is essentially the init call.

Unless of course there are complications due to threads...

Are you proposing to move the all and only the problematic thread-unsafe stuff in new() (to be called per thread) and keeping all the rest in a new sub init(), to be called in the BEGIN block to initialize the $instance singleton var?

[fpl]

@shawnlaffan It would be nice if someone could review my proposal to split the initialization code into two parts, one for the BEGIN section and one to be called per thread. Of course it would represent a change in the current API, but I can't see any other way to ensure thread safety.

[shawnlaffan]

The issue is that the changes break the current API and now non-threaded systems do not have an error handler set by default.

I wonder if the new call should generate an object that keeps track of which thread it is in. Then it can set the error handler if the thread field is undef. That might need a lot more engineering, though, depending on how GDAL does things.

[fpl]

The issue is that the changes break the current API and now non-threaded systems do not have an error handler set by default.

I wonder if the new call should generate an object that keeps track of which thread it is in. Then it can set the error handler if the thread field is undef. That might need a lot more engineering, though, depending on how GDAL does things.

That's my fault: RTFM applies as often in a dev life.

I think a better approach would be de-registering the error handler in a CLONE_SKIP sub and re-registering per thread in a CLONE sub. That would render the module thread-safe and avoid the need to change API. Ok, I'm a silly dumb, guys...
I'll change the PR and make some checks, but this approach seems the most reasonable.

[fpl]

Ok, this PR uses a CLONE_SKIP to disable GDAL error handling in the main thread, just before creating a new thread, then re-enables it in the created thread after within a CLONE sub. The existing code requires no API change if it is a single-thread application. The only change is that in multi-threaded apps (which never worked until now), the main thread needs to re-enable GDAL error handling before using GDAL and after the creation of all its threads (CLONE_SKIP is called at every thread creation). An alternative would be returning 1 in CALL_SKIP and forcing the creation of a new() instance in every thread before using GDAL. I have a slight preference for this approach instead, because it is less impacting on performance.

[shawnlaffan]

This passes tests on a local perl 5.38.2 built with ithreads.

[fpl]

I would add a note into the FFI pod about re-enabling error handling in the main thread after thread pool creation, if acceptable. That would complete the PR.

[fpl]

Add a note in the FFI.pm POD section as said.

[fpl]

Hi there, any hope to see this PR considered for next release? This patch is much less intrusive and solves the support of multi-threading app without big problems for existing code (which I guess do not use threads, else it would simply not work at all, as was mine). Even, the drop of the obsolete StringPointer module using is simply a due act and solves even memory leaks under the same MTs conditions and possibly other situations.
Thanks for reviewing, just in case.

[ajolma]

Sure, it's just that I don't actively work on this and have no regular timeslots dedicated for this. @shawnlaffan, I believe you have full rights on this repo and on this distribution on CPAN, so you can make releases too.

[shawnlaffan]

@ajolma - I have commit privileges to this repo but I don't think I have PAUSE access to upload to CPAN.

[shawnlaffan]

@fpl - I've added some nit-picky comments to the non-code components above. I'll have a closer look at the code itself soon.

[ajolma]

@ajolma - I have commit privileges to this repo but I don't think I have PAUSE access to upload to CPAN.

You're a co-maintainer of Geo::GDAL::FFI* modules.

[shawnlaffan]

@fpl - the code looks good but unfortunately the recent merger of #70 is now causing merge conflicts.

Are you able to resolve them? Ideally the commits in this PR would also be rebased on the current master branch and force-pushed but I can also do that at merge time if needed.

[fpl]

Ok, I rebased on master, and my branch is now c.lean. Unfortunately I have no write-access here, so you should solve the remaining conflict yourself, it is immediate.

[shawnlaffan]

Thanks @fpl - changes merged. I'll make a few other changes and cut a new release to CPAN.