run gdalallregister during module initialisation

[mathieu17g]

@visr as discussed in yeesian/ArchGDAL.jl#245 (comment) here is a PR proposition for comments.

I have kept commented the registration at exit of cplseterrorhandler(C_NULL) and of a trial wih gdaldumpopendatasets for easy revert for the former and comment for the latter

[visr]

Oh it is nice if indeed gdaldestroy will take care of everything. Here is the code and docstring for it: https://github.com/OSGeo/gdal/blob/3c518a6ca/gdal/gcore/gdaldllmain.cpp#L55-L100

For the dump open datasets function, I hope we can use it to test the issues that we were seeing, so we make sure that gdaldestroy does what we expect. It would be nice to have such a test in GDAL.jl and not only in ArchGDAL.jl.

To put the dump open datasets function in the init function, I'm wondering if it is a good idea. For proper applications you want to always close things, but I'm doubting if we should print this for interactive users that may just want to try out some things.

So gdaldestroy doesn't close the open datasets?

[yeesian]

So gdaldestroy doesn't close the open datasets?

To my understanding (see links below), it does:

• https://trac.osgeo.org/gdal/ticket/3954
• https://github.com/OSGeo/gdal/blob/fec15b146f8a750c23c5e765cac12ed5fc9c2b85/gdal/gcore/gdaldllmain.cpp#L86
• https://github.com/OSGeo/gdal/blob/939321f91aefb8b78b07080212b77bdba31d44da/gdal/gcore/gdaldrivermanager.cpp#L153-L335

[visr]

Ah good, glad it also closes open datasets. So regarding this PR, I'm happy to have it. Though there are two points I still want to discuss:

• Should we move gdalallregister() back into GDAL.jl? All other GDAL config and initialisation is here, so maybe this should too?
• Can we test the effect of gdaldestroy here in some way? Testing the effect of atexit is probably difficult. We could just run gdaldestroy ourselves, but then we effectively brick our session, right? If tests are not feasible then so be it.

[yeesian]

Should we move gdalallregister() back into GDAL.jl? All other GDAL config and initialisation is here, so maybe this should too?

Do we know which initializations are heavyweight and which ones are not? Maybe this should be tracked in a ticket for discussion. I'm personally okay with it, but I'm not a particularly sophisticated user of GDAL

[visr]

@mathieu17g note that making modifications to epilogue.jl, or anything under the gen folder, has no direct effect. Only if you re-run the wrapper script, or manually update the GDAL.jl file, will it make a difference. In bb6e4ad I added this.

I didn't think about this in #123, so that PR didn't change any behavior. Given that, do you still think deregistering the error handlers should be done, or is that covered by gdaldestroy as well?

[mathieu17g]

@mathieu17g note that making modifications to epilogue.jl, or anything under the gen folder, has no direct effect. Only if you re-run the wrapper script, or manually update the GDAL.jl file, will it make a difference. In bb6e4ad I added this.

I didn't think about this in #123, so that PR didn't change any behavior. Given that, do you still think deregistering the error handlers should be done, or is that covered by gdaldestroy as well?

OK, then since my previous test results beyond commit yeesian/ArchGDAL.jl@95f6274 in PR yeesian/ArchGDAL.jl#245, are jeopardized.

I have tested ArchGDAL.jl v0.7.4 against GDAL.jl v1.2.3 with the combinations of the following additions in GDAL.jl/src/GDAL.jl : atexit(gdaldestroy) and/or atexit(() -> cplseterrorhandler(C_NULL)).
All combinations raise an error at exit when using Pkg.test("ArchGDAL")

Therefore, I will restart testing from commit yeesian/ArchGDAL.jl@95f6274 (against GDAL.jl v1.2.3) when I modified the DriverManager constructor in ArchGDAL.jl/src/ArchGDAL.jl

[mathieu17g]

@visr and @yeesian here are my thoughts on the topic. Sorry it's a bit long.

The current implementation of __init__() in ArchGDAL.jl v0.7.4 is:

mutable struct DriverManager
    function DriverManager()
        drivermanager = new()
        GDAL.gdalallregister()
        finalizer((dm,) -> GDAL.gdaldestroydrivermanager(), drivermanager)
        return drivermanager
    end
end

const DRIVER_MANAGER = Ref{DriverManager}()

function __init__()
    DRIVER_MANAGER[] = DriverManager()
    return nothing
end

1. The error at Julia exit, exclusively comes from the call to GDAL.destroydrivermanager

With GDAL.jl v1.2.3, it raises an error at Julia exit. I had the hunch in yeesian/ArchGDAL.jl#245 (comment) that is was linked to the DriverManager finalization and I sloppily jumped in yeesian/ArchGDAL.jl#245 (review) to the conclusion that the problem was solved by deregistering the Julia custom error handler.

If we make a few test with Pkg.test("ArchGDAL") around this lead by modifying __init__() in ArchGDAL.jl/src/ArchGDAL.jl:

• With

  function __init__()
      GDAL.gdalallregister()
      return nothing
  end

  -> no error

• With

  function __init__()
      GDAL.gdalallregister()
      atexit(GDAL.destroydrivermanager())
      return nothing
  end

  -> error

• With

  function __init__()
      GDAL.gdalallregister()
      atexit(GDAL.destroydrivermanager())
      atexit(() -> GDAL.cplseterrorhandler(C_NULL))
      return nothing
  end

  -> error and idem if the two exit hooks are swapped

• With

  function __init__()
      GDAL.gdalallregister()
      atexit(() -> GDAL.cplseterrorhandler(C_NULL))
      return nothing
  end

  -> no error

• With

  function __init__()
      GDAL.gdalallregister()
      atexit(GDAL.gdaldestroy)
      return nothing
  end

  -> error

1st conclusion

• The error has to do with the call of GDAL.gdaldestroydrivermanager
• Unregistering the Julia custom error handler has nothing to do with the error suppression
• GDAL.gdaldestroy does not help

2. There is no need to map GDAL GDALDriverManager in ArchGDAL.jl, and therefore no need to use GDAL.destroydrivermanager as a finalizer

To keep @yeesian intent to create a Julia mapping of GDAL DriverManager, and noticing in yeesian/ArchGDAL.jl#245 (review) that the current implementation does not hold any actual link between Julia struct an GDAL DriverManager, I tried to enhance the mapping in Julia of the GDAL DriverManager object:

mutable struct DriverManager
    ptr::GDAL.GDALMajorObjectH

    function DriverManager(ptr::GDAL.GDALMajorObjectH = C_NULL)
        drivermanager = new(ptr)
        finalizer(destroy, drivermanager)
        return drivermanager
    end
end

function destroy(dm::DriverManager)
    GDAL.gdaldestroydrivermanager()
    dm.ptr = C_NULL
    return nothing
end

const DRIVER_MANAGER = Ref{DriverManager}()

function __init__()
    DRIVER_MANAGER[] = DriverManager(GDAL.getgdaldrivermanager())
    # GDAL.gdalallregister()
    return nothing
end

The recommended way to get a pointer to GDAL GDALDriverManager is to use GDAL.getgdaldrivermanager().

• Unfortunately, it is not available in GDAL.jl/src/GDAL.jl
• In gdal/gdal/gcore/gdaldrivermanager.cpp GDALGetDriverManager() does not have the CPL_STDCALL tag as other available functions of gdal/gdal/gcore/gdaldrivermanager.cpp.
• In gdal/gdal/gcore/gdal_priv.h GDALGetDriverManager() has the CPL_DLL tag
• But since my knowledge to C++ and Clang.jl is very limited, I cannot figure if it would be feasible and/or advisable to make GDALGetDriverManager() available in GDAL.jl/src/GDAL.jl. @visr, it up to you

Anyway, is there any other usage of GDALGetDriverManager() for a user ?

• If we search for its usage in gdal.org, here are the occurrences:
  • In the GNM API Tutorial, this part of GDAL has no C API
  • In the Raster API tutorial and Vector API tutorial but only for the C++ part in examples
  • In the Raster driver implementation tutorial and Vector driver implementation tutorial but there is no such usage in GDAL.jl and ArchGDAL.jl.

2nd conclusion

We can safely drop the need of mapping GDAL GDALDriverManager object in ArchGDAL

3. No impact identified of not calling GDAL.gdaldestroydrivermanager at all

This leaves @visr's question in yeesian/ArchGDAL.jl#245 (comment):

But why does this PR remove the call to GDAL.gdaldestroydrivermanager()? Isn't that the counterpart to GDAL.gdalallregister() that is called when ArchGDAL loads? Should gdaldestroydrivermanager be called onexit here?

Well no idea, but there is no error.
Do you fear that there would be some leakage beyond Julia exit ?
And if so, do you know anyway to check it ?

3rd conclusion

If not evidence can be found of a negative effect of not calling GDAL.destroydrivermanager anywhere after having called GDAL.gdalallregister, I suggest to:

• Revert GDAL.jl to v1.2.3
• Keep only an __init__() function in ArchGDAL.jl/src/ArchGDAL.jl containing GDAL.gdalallregister()

@visr and @yeesian, what do you think?

[visr]

Thanks again for a thorough analysis. Some replies:

But since my knowledge to C++ and Clang.jl is very limited, I cannot figure if it would be feasible and/or advisable to make GDALGetDriverManager() available in GDAL.jl/src/GDAL.jl. @visr, it up to you

I guess it is not currently not wrapped since it is in a file called gdal_priv.h, which reads to me like it is not meant for public use? But the function is present in the shared library, this works:

julia> using GDAL, GDAL_jll

julia> drivermanager = ccall((:GetGDALDriverManager, libgdal), Ptr{Cvoid}, ())
Ptr{Nothing} @0x000000007c92ac20

Do you fear that there would be some leakage beyond Julia exit ?
And if so, do you know anyway to check it ?

I don't fully understand, but I guess most things will be fine after julia exit. I don't know if there are concrete examples where atexit(gdaldestroy) would be better then just exiting. Though there may well be, atexit(gdaldestoy) is giving GDAL a chance to clean things up properly.

What kind of tests did you run to determine error or no error? Running the whole ArchGDAL test suite? I prefer minimal cases, ideally GDAL.jl only, that allow us to understand the functions. I just adapted this from the test/tutorial_raster.jl file. It may be relevant that I ran this under Windows.

using GDAL

utmsmall = joinpath(@__DIR__, "data/utmsmall.tif")

GDAL.gdalallregister()
dataset = GDAL.gdalopen(utmsmall, GDAL.GA_ReadOnly)

rm(utmsmall)  # IOError because it is open (can remove it after closing julia session)

GDAL.gdaldestroy()
rm(utmsmall)  # now it works I can delete the file since it is no longer open thanks to gdaldestroy

This example makes sense to me. Can we craft something similar in which gdaldestroy throws an error?

Do we know which initializations are heavyweight and which ones are not?

@yeesian on first run GDAL.gdalallregister() finishes in 3ms for me

[mathieu17g]

What kind of tests did you run to determine error or no error? Running the whole ArchGDAL test suite?

Yes, with the sequence

1. Modification of __init__() in ArchGDAL.jl/src/ArchGDAL.jl
2. Pkg.update()
3. Pkg.test("ArchGDAL")

[yeesian]

on first run GDAL.gdalallregister() finishes in 3ms for me

@visr yeah I've always experienced it to be fast for myself too. If it matters, my vote's for it to run by default :)

[yeesian]

I wasn't able to reproduce the IOError when following the example in #124 (comment) in a M1 Macbook:

  | | |_| | | | (_| |  |  Version 1.6.0 (2021-03-24)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

shell> pwd
/Users/yeesian/.julia/dev/GDAL/test

julia> using GDAL

julia> utmsmall = joinpath(@__DIR__, "data/utmsmall.tif")
"/Users/yeesian/.julia/dev/GDAL/test/data/utmsmall.tif"

julia> GDAL.gdalallregister()

julia> dataset = GDAL.gdalopen(utmsmall, GDAL.GA_ReadOnly)
Ptr{Nothing} @0x00007fb7528ce390

julia> rm(utmsmall)

However, I consistently get issues with GDAL.gdaldestroy() without having any issues with GDAL.gdaldestroydrivermanager():

  | | |_| | | | (_| |  |  Version 1.6.0 (2021-03-24)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> import GDAL

julia> GDAL.gdalallregister()

julia> GDAL.gdaldestroydrivermanager()

julia> GDAL.gdaldestroy()
CPLGetTLSList(): pthread_setspecific() failed!

  | | |_| | | | (_| |  |  Version 1.6.0 (2021-03-24)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> import GDAL

julia> GDAL.gdalallregister()

julia> GDAL.gdaldestroy()
CPLGetTLSList(): pthread_setspecific() failed!

(Revisiting #18: although it is written in https://sgillies.net/2014/01/22/rethinking-driver-management-in-fiona-and-rasterio.html, I have not seen GDALDestroyDriverManager() being used in rasterio and Fiona even though (i) GDALAllRegister() is still being used in rasterio and Fiona and (ii) it had existed in that codebase in the past.)

[visr]

I wasn't able to reproduce the IOError when following the example in #124 (comment) in a M1 Macbook

Ah that is probably just a OS filesystem difference, I think Windows locks open files.

Strange that you get errors on gdaldestroy. But that together with seeing what they do in fiona and rasterio, leads me to think that we should have gdalregisterall in the GDAL.jl init, and no destroy*, for now at least. Running gdalregisterall multiple times is harmless, so adding it here before removing it from ArchGDAL should be fine.

How's this PR with the latest commit?