Opened 13 years ago

Closed 13 years ago

#1635 closed defect (fixed)

File handle leak when reading s57 files.

Reported by: blackmoreb Owned by: warmerdam
Priority: normal Milestone:
Component: OGR_SF Version: 1.4.1
Severity: normal Keywords: csharp s57
Cc: tamas, hobu

Description

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.

Attachments (8)

GR4APP08.000 (289.1 KB) - added by blackmoreb 13 years ago.
GR4APP08.001 (5.7 KB) - added by blackmoreb 13 years ago.
GR4APP08.002 (2.0 KB) - added by blackmoreb 13 years ago.
GR4APP08.003 (2.9 KB) - added by blackmoreb 13 years ago.
GR4APP08.004 (2.8 KB) - added by blackmoreb 13 years ago.
ogrinfo.cs (2.7 KB) - added by blackmoreb 13 years ago.
gdal_gdal_csharp.2.dll (56.0 KB) - added by blackmoreb 13 years ago.
gdal_gdal_csharp.dll (56.0 KB) - added by blackmoreb 13 years ago.

Download all attachments as: .zip

Change History (29)

Changed 13 years ago by blackmoreb

Attachment: GR4APP08.000 added

Changed 13 years ago by blackmoreb

Attachment: GR4APP08.001 added

Changed 13 years ago by blackmoreb

Attachment: GR4APP08.002 added

Changed 13 years ago by blackmoreb

Attachment: GR4APP08.003 added

Changed 13 years ago by blackmoreb

Attachment: GR4APP08.004 added

comment:1 Changed 13 years ago by blackmoreb

Owner: changed from warmerda to warmerdam

comment:2 Changed 13 years ago by blackmoreb

Version: unspecified1.4.1

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.

comment:3 Changed 13 years ago by warmerdam

Cc: tamas added
Component: OGR_SRSOGR_SF
Keywords: csharp s57 added

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.

comment:4 in reply to:  3 Changed 13 years ago by 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

comment:5 Changed 13 years ago by 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

Changed 13 years ago by blackmoreb

Attachment: ogrinfo.cs added

comment:6 Changed 13 years ago by 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.

comment:7 Changed 13 years ago by warmerdam

Cc: hobu added

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.

comment:8 Changed 13 years ago by 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?

comment:9 Changed 13 years ago by 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.

Changed 13 years ago by blackmoreb

Attachment: gdal_gdal_csharp.2.dll added

Changed 13 years ago by blackmoreb

Attachment: gdal_gdal_csharp.dll added

comment:10 Changed 13 years ago by 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.

comment:11 Changed 13 years ago by tamas

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

Thanks,

Tamas

comment:12 Changed 13 years ago by 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.

comment:13 Changed 13 years ago by 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.

comment:14 Changed 13 years ago by 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.

comment:15 Changed 13 years ago by 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.

comment:16 Changed 13 years ago by 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.

comment:17 Changed 13 years ago by 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

comment:18 Changed 13 years ago by 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.

comment:19 in reply to:  18 Changed 13 years ago by 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.

comment:20 Changed 13 years ago by warmerdam

Status: newassigned

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

comment:21 Changed 13 years ago by warmerdam

Resolution: fixed
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.