CPLErrorV change to better support scriptland

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

CPLErrorV change to better support scriptland

Kevin Ruland
Frank,

One of the things Shawn expressed dissatisfaction from was for some
problems, gdal will abort() and the interpreter would go away very
quickly.  This of course, is not very nice behavour.

The problem is very apparent in the NG swig code because I don't do any
special parameter checking prior to calling the underlying method.  So
for example, say "out.tif" only contains one layer, then if I do this:

f = gdal.Open( 'out.tif' )
f.GetRasterBand(42)

The underlying GDALDataset::GetRasterBand() gets the numer 42 with no
prior checking.  It rightfually decides this is a good time to issue a
CPLError( CE_Fatal,....).  The problem is CPLErrorV will call abort() if
the error class is CE_Fatal.  Bye bye python.

I see that in the old bindings, you place extra checks in the script
side for the parameters. This is not very amenable to supporting
multiple bindings.

Is there any way to remove this abort() entirely and leave it up to the
application to decide if a problem is serious enough to abort?  It just
seems more polite but I don't understand what the implications would be.

Kevin


_______________________________________________
Gdal-dev mailing list
[hidden email]
http://xserve.flids.com/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: CPLErrorV change to better support scriptland

Frank Warmerdam-2
On 6/19/05, Kevin Ruland <[hidden email]> wrote:

> Frank,
>
> One of the things Shawn expressed dissatisfaction from was for some
> problems, gdal will abort() and the interpreter would go away very
> quickly.  This of course, is not very nice behavour.
>
> The problem is very apparent in the NG swig code because I don't do any
> special parameter checking prior to calling the underlying method.  So
> for example, say "out.tif" only contains one layer, then if I do this:
>
> f = gdal.Open( 'out.tif' )
> f.GetRasterBand(42)
>
> The underlying GDALDataset::GetRasterBand() gets the numer 42 with no
> prior checking.  It rightfually decides this is a good time to issue a
> CPLError( CE_Fatal,....).  The problem is CPLErrorV will call abort() if
> the error class is CE_Fatal.  Bye bye python.
>
> I see that in the old bindings, you place extra checks in the script
> side for the parameters. This is not very amenable to supporting
> multiple bindings.
>
> Is there any way to remove this abort() entirely and leave it up to the
> application to decide if a problem is serious enough to abort?  It just
> seems more polite but I don't understand what the implications would be.

Kevin,

I'm not really keen on removing the abort() for fatal errors, but
I do think that this particular error should be changed to just
a normal CE_Failure error intsead of a fatal error.  Really the
only use I see currently for the fatal error is when CPLMalloc()
fails in an allocation.  

I will change GDALDataset::GetRasterBand() to issue a
CE_Failure error and return NULL in case of an illegal input.
This can be then translated into an appropriate type of exception
in python and possibly other languages.

Best regards,
--
---------------------------------------+--------------------------------------
I set the clouds in motion - turn up   | Frank Warmerdam, [hidden email]
light and sound - activate the windows | http://pobox.com/~warmerdam
and watch the world go round - Rush    | Geospatial Programmer for Rent
_______________________________________________
Gdal-dev mailing list
[hidden email]
http://xserve.flids.com/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: CPLErrorV change to better support scriptland

Kevin Ruland

Frank:

Fair enough solution.

But I think I could trump the whole thing and let you do what you need
to do.  Suppose in language binding X (where X = python :) I write my
own special little ErrorHandler.  And further this ErrorHandler does not
return to the caller (CPLErrorV) but rather throws.  Then in the Swig
binding world, I catch this throw and convert into an appropriate
exception in language X.  Of course, there are some issues:  1)  Need to
have swig generated try blocks around *every* native call.  2) I don't
know how this would interact with user supplied error handlers.  The
user supplied error handler would have to raise/throw/whatever in order
to trump the abort().  This might be fairly simple to do in the ScriptX
<-> ErrorHandler mapping wrapper, but there could be some rather wierd
issues.

Kevin

Frank Warmerdam wrote:

>On 6/19/05, Kevin Ruland <[hidden email]> wrote:
>  
>
>>Frank,
>>
>>One of the things Shawn expressed dissatisfaction from was for some
>>problems, gdal will abort() and the interpreter would go away very
>>quickly.  This of course, is not very nice behavour.
>>
>>The problem is very apparent in the NG swig code because I don't do any
>>special parameter checking prior to calling the underlying method.  So
>>for example, say "out.tif" only contains one layer, then if I do this:
>>
>>f = gdal.Open( 'out.tif' )
>>f.GetRasterBand(42)
>>
>>The underlying GDALDataset::GetRasterBand() gets the numer 42 with no
>>prior checking.  It rightfually decides this is a good time to issue a
>>CPLError( CE_Fatal,....).  The problem is CPLErrorV will call abort() if
>>the error class is CE_Fatal.  Bye bye python.
>>
>>I see that in the old bindings, you place extra checks in the script
>>side for the parameters. This is not very amenable to supporting
>>multiple bindings.
>>
>>Is there any way to remove this abort() entirely and leave it up to the
>>application to decide if a problem is serious enough to abort?  It just
>>seems more polite but I don't understand what the implications would be.
>>    
>>
>
>Kevin,
>
>I'm not really keen on removing the abort() for fatal errors, but
>I do think that this particular error should be changed to just
>a normal CE_Failure error intsead of a fatal error.  Really the
>only use I see currently for the fatal error is when CPLMalloc()
>fails in an allocation.  
>
>I will change GDALDataset::GetRasterBand() to issue a
>CE_Failure error and return NULL in case of an illegal input.
>This can be then translated into an appropriate type of exception
>in python and possibly other languages.
>
>Best regards,
>  
>
_______________________________________________
Gdal-dev mailing list
[hidden email]
http://xserve.flids.com/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: CPLErrorV change to better support scriptland

Frank Warmerdam-2
On 6/20/05, Kevin Ruland <[hidden email]> wrote:

>
> Frank:
>
> Fair enough solution.
>
> But I think I could trump the whole thing and let you do what you need
> to do.  Suppose in language binding X (where X = python :) I write my
> own special little ErrorHandler.  And further this ErrorHandler does not
> return to the caller (CPLErrorV) but rather throws.  Then in the Swig
> binding world, I catch this throw and convert into an appropriate
> exception in language X.  Of course, there are some issues:  1)  Need to
> have swig generated try blocks around *every* native call.  2) I don't
> know how this would interact with user supplied error handlers.  The
> user supplied error handler would have to raise/throw/whatever in order
> to trump the abort().  This might be fairly simple to do in the ScriptX
> <-> ErrorHandler mapping wrapper, but there could be some rather wierd
> issues.

Kevin,

Certainly the error handlers installed can choose to do something
other than returning if the error class is CE_Fatal.  But keep in mind
that the error class is set to fatal because continued execution on
the main code path is not possible.  GDAL datastructures are likely
to be arbitrarily invalid after such an error.  

PS. I have committed the patch to remove the fatal error in
the GetRasterBand() method.  

Best regards,
--
---------------------------------------+--------------------------------------
I set the clouds in motion - turn up   | Frank Warmerdam, [hidden email]
light and sound - activate the windows | http://pobox.com/~warmerdam
and watch the world go round - Rush    | Geospatial Programmer for Rent
_______________________________________________
Gdal-dev mailing list
[hidden email]
http://xserve.flids.com/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: CPLErrorV change to better support scriptland

Kevin Ruland

>Kevin,
>
>Certainly the error handlers installed can choose to do something
>other than returning if the error class is CE_Fatal.  But keep in mind
>that the error class is set to fatal because continued execution on
>the main code path is not possible.  GDAL datastructures are likely
>to be arbitrarily invalid after such an error.  
>
>  
>

Frank.

That would certainly be true and it would be up to the programmer to
make sure they didn't do anything stupid, like ignore the exception.  In
the event of such an error, the script might have some important work to
do such as return an error page (in cgi applications) or close databases
or start a pot of coffee for a late night debugging session and the
premature abort() would prevent any action.

Actually, if the error was from malloc, there could be quite a bit of
invalidation throughout the interpreter.  Maybe this wouldn't be such a
good idea.

Kevin


_______________________________________________
Gdal-dev mailing list
[hidden email]
http://xserve.flids.com/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: CPLErrorV change to better support scriptland

Frank Warmerdam-2
On 6/20/05, Kevin Ruland <[hidden email]> wrote:
> Frank.
>
> That would certainly be true and it would be up to the programmer to
> make sure they didn't do anything stupid, like ignore the exception.  In
> the event of such an error, the script might have some important work to
> do such as return an error page (in cgi applications) or close databases
> or start a pot of coffee for a late night debugging session and the
> premature abort() would prevent any action.

Kevin,

Right.  However, in my experience, with the GetRasterBand() case
set aside, abort() fatal errors are very rare.  It only really happens
if CPLMalloc() fails and that only normally happens if a size calculation
goes wonky.  Large allocations are normally done via VSIMalloc()
(a direct wrapper) and in those cases the caller is expected to recover
gracefully from a failed allocation.
 
> Actually, if the error was from malloc, there could be quite a bit of
> invalidation throughout the interpreter.  Maybe this wouldn't be such a
> good idea.

Well, safe recover might still be difficult.  My suggestion is not to
worry about this aspect much at this time as I don't think it will arise
except in the worst of circumstances in the future.

Best regards,
--
---------------------------------------+--------------------------------------
I set the clouds in motion - turn up   | Frank Warmerdam, [hidden email]
light and sound - activate the windows | http://pobox.com/~warmerdam
and watch the world go round - Rush    | Geospatial Programmer for Rent
_______________________________________________
Gdal-dev mailing list
[hidden email]
http://xserve.flids.com/mailman/listinfo/gdal-dev