File handle leak when reading s57 files.

[blackmoreb]

When reading certain S57 files the OGR layer throws legitimate exceptions but may be holding onto file handles. This stops files from being deleted unless the host application is first closed down.

I have included the base and updates for the files in question.

[blackmoreb]

The exceptions thrown are “Data record is short on DDF file” and ‘ISO8211 record leader appears to be corrupt.’. If you then try and delete the files from the same process an exception is thrown as the OGR layer seems to be holding onto the file handles.

[warmerdam]

Ben,

Do the errors occur at the point you try to open the dataset? I have tested the dataset with ogrinfo, and it does not seem to leave any files open. But then ogrinfo gets the chance to properly close the dataset.

In fact, despite the following error message, ogrinfo does not seem to have much problem reading the dataset.

ERROR 1: ISO8211 record leader appears to be corrupt.

I do notice a number of debug messages like:

ISO8211: Didn't find field terminator, read one more byte.

Since I believe this issue may be related to SWIG C# exception generation, I am adding Tamas as a cc. In addition to details on where you get the exception, it might be helpful to see the local snippet of code from around that point.

[blackmoreb]

Replying to warmerdam:

Ben,

Do the errors occur at the point you try to open the dataset? I have tested the dataset with ogrinfo, and it does not seem to leave any files open. But then ogrinfo gets the chance to properly close the dataset.

In fact, despite the following error message, ogrinfo does not seem to have much problem reading the dataset.

ERROR 1: ISO8211 record leader appears to be corrupt.

I do notice a number of debug messages like:

ISO8211: Didn't find field terminator, read one more byte.

Since I believe this issue may be related to SWIG C# exception generation, I am adding Tamas as a cc. In addition to details on where you get the exception, it might be helpful to see the local snippet of code from around that point.

Frank, Tamas

I have attached a modified version of ogrinfo (takes a path instead of a file) that I am using as a test app. The problem occurs during the ogr.Open(file.FullName, 0) this usually results in on of the mentioned exceptions. I then find that when i get to the Directory.Delete(args[0], true) another exception is thrown because the files are locked.

Hope this helps.

Ben

[blackmoreb]

More testing. More Results.

It would seem the behaviour I have described is totaly dependant on the existence of the CSV files in the app path or at the location sepecified by S57_CSV. Once I introduce these files into the app path of my modified ogrinfo.exe then the errors occur as I describe. It also helps if you have the correct version of my ogrinfo.cs (I posted the wrong one, and have now replaced it.) I am confident now that if you use my ogrinfo.cs with the CSV files in the app path and the data I supplied you will get the "Data record is short on DDF file" and not be able to delete the file.

Regards

Ben

[warmerdam]

I have tried to reproduce a similar problem with the python bindings with exceptions enabled, but it seems the python is quite different.

I believe the issue is that the Open() does not destroy the returned datasource at the point where it throws an exception, though clearly it needs to. I'm going to wait on feedback from Tamas.

[warmerdam]

After hobu fixed #1638, the same issue occurs with python.

So it is fairly generally an issue with how we are handling swig exceptions.

hobu added to cc list.

[tamas]

Ben,

I could not reproduce the problem you've reported. I've tested the code with both the svn-trunk ad the branch-1.4 but I did not got any errors.

Could you post your complete project to me including the dll-s you've used?

[blackmoreb]

tamas

As requested I have zipped up my app dir and posted it. It includes the VS2005 project and S57 files.

Regards & Thanks

Ben.

[blackmoreb]

tamas I gave up trying to send the files through the site, I have emailed them to Frank and asked him to forward them to you.

Thanks for your help.

Ben.

[tamas]

I think we have a limit of the attachment size on the trac. You can also send it directly to szekerest@….

Thanks,

Tamas

[tamas]

Ben,

You should call the Dataset.Dispose explicitly before trying to remove the files

using (Driver drv = ds.GetDriver()) {

if (drv == null) {

Console.WriteLine("Can't get driver.");

} Console.WriteLine("Using driver " + drv.name); Layer coverage = ds.GetLayerByName("M_COVR"); if (coverage != null) {

Console.WriteLine(coverage.GetName());

}

} ds.Dispose();

Otherwise the file handle will not be closed until the garbage collector destroys this object.

[blackmoreb]

Thomas I have mailed the files to you as requested.

The problem for me is that the exception occurs on the .Open() line and therefore the dataset object is null.

Let me know if you need any more information.

Regards

Ben.

[tamas]

I've posted a message to the swig-user list with this issue. The default exception handler of some languages (not only for the C#) put up the hands and return with null instead of the object reference when error happens.

This behaviour could easily be altered by the redefinition of SWIG_exception of these languages, however I cannot see the potential issues may be caused by applying this change. At this time I leave the swig folks to find out the most convenient treatment for this problem.

May be we can also handle these problems locally by categorizing the CPLErr codes in seriousness, and alter %exception in cpl_exceptions.i not to call SWIG_exception in these cases.

[blackmoreb]

That sounds great Tamas.

Is there anything you can think of I can try as a work around for this issue ? I have tried quite a few things but nothing seems to have the desired effect.

Let me know if there is anything else I can do to help.

Regards

Ben.

[tamas]

I think we might replace CE_Failure with CE_Warning in ddfrecord.cpp line 294 But I'll leave this change to Frank since there are some other places where a similar effort should be done.

[blackmoreb]

Frank, Tamas

Thanks for all your help. I have tried the code change put forward by Tamas and it does get me past my immediate issue. I dont have a feel yet for any possible knock on effects but I will let you know if I experience any weirdness.

Regards

Ben

[warmerdam]

I am thinking that this code in ogr.i:

  OGRDataSourceShadow* Open( const char *filename, int update =0 ) {
    OGRDataSourceShadow* ds = (OGRDataSourceShadow*)OGROpen(filename,update,NULL);
    return ds;
  }

should be changed to:

  OGRDataSourceShadow* Open( const char *filename, int update =0 ) {
    CPLErrorReset();
    OGRDataSourceShadow* ds = (OGRDataSourceShadow*)OGROpen(filename,update,NULL);
    if( CPLGetLastErrorType() == CE_Failure )
    {
        OGRReleaseDataSource(ds);
        ds = NULL;
    }

    return ds;
  }

I think I may also need to tune down the error within the S-57 driver, but I'd prefer to address the datasource leak at the swig level first.

[tamas]

Replying to warmerdam:

I am thinking that this code in ogr.i:

  OGRDataSourceShadow* Open( const char *filename, int update =0 ) {
    OGRDataSourceShadow* ds = (OGRDataSourceShadow*)OGROpen(filename,update,NULL);
    return ds;
  }

should be changed to:

  OGRDataSourceShadow* Open( const char *filename, int update =0 ) {
    CPLErrorReset();
    OGRDataSourceShadow* ds = (OGRDataSourceShadow*)OGROpen(filename,update,NULL);
    if( CPLGetLastErrorType() == CE_Failure )
    {
        OGRReleaseDataSource(ds);
        ds = NULL;
    }

    return ds;
  }

I think I may also need to tune down the error within the S-57 driver, but I'd prefer to address the datasource leak at the swig level first.

I'm +1 to this change along with the replacement of the CE_Failure with CE_Warning in this particular case. We might also look over the code and find the other places where a similar change should be made. And also the CE_Failure --> CE_Warning transition might apply in other places as well like ceosopen.c line 111 for example.

[warmerdam]

Change applied for ogr and gdal open (r11529 and r11531).

[warmerdam]

I reviewed the low level error being reported and decided that it is legitimate, and represents a failure to be able to apply the updates properly. I therefore modified the S-57 code to preserve and propagate the error up the chain better and now the OGROpen() no longer returns a dataset.

This change (r11537) is only in trunk - too scary to backport.