search

JuliaGeo / GDAL.jl

Thin Julia wrapper for GDAL - Geospatial Data Abstraction Library
MIT License
90 stars 13 forks source link

remove throw from gdaljl_errorhandler #153

Closed maxfreu closed 1 year ago

maxfreu commented 1 year ago

The throw interrupts control flow on the C side, e.g. suppressing the closing of file descriptors when the error is caught by julia. This PR removes it, but leaves the error handler in place, so that printing by GDAL is suppressed. Furthermore, maybe_throw now also throws on fatal errors. Locally, all test pass.

ref https://github.com/rafaqz/Rasters.jl/issues/455

maxfreu commented 1 year ago

Oh, before I forget: There is an alternative how errors could be handled. One could have a global const ref to a GDALError that gets passed as user data to the registered error handler function and gets updated inside. This would require registering the gdaljl_errorhandler at load time I think, because the address the ref points to will always be different. But all this sounds bad and should only be considered if it is really necessary.

visr commented 1 year ago

Yeah that sounds like a worse option. I'm ok with this, I'll leave it open for a bit to give people a chance to react.

rafaqz commented 1 year ago

Do we lose anything losing this error printing? Or things will print anyway from maybe_throw() ?

felixcremer commented 1 year ago

If we loose the error printing, we could at least log the failure number and error message.

visr commented 1 year ago

We don't lose the error printing with this. The gdaljl_errorhandler threw GDALErrors from C. This approach does not provide us with a way to clean up things on the julia side, or to customize the behavior for different functions such as IO. We already have a system in place to check for errors after a ccall. That doesn't have those limitations, and with it we actually don't need the gdaljl_errorhandler to do anything other than ignoring those calls from C.

I think the reason I kept both systems in place was because I wasn't sure if the check from the julia side would catch all the errors that the error handler catches. But I don't know any examples of that.

Another thing that I can imagine that one ccall sets first one, more informative error, and then another. In this new approach we would no longer see the first, but only the last. Though I haven't seen any examples of this either. I guess generally on CE_Failure or CE_Fatal the ccall would return anyway.

rafaqz commented 1 year ago

Makes sense, we can live with just the last error. Im happy with this then.