Opened 17 years ago
Closed 17 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)
Change History (29)
by , 17 years ago
Attachment: | GR4APP08.000 added |
---|
by , 17 years ago
Attachment: | GR4APP08.001 added |
---|
by , 17 years ago
Attachment: | GR4APP08.002 added |
---|
by , 17 years ago
Attachment: | GR4APP08.003 added |
---|
by , 17 years ago
Attachment: | GR4APP08.004 added |
---|
comment:1 by , 17 years ago
Owner: | changed from | to
---|
comment:2 by , 17 years ago
Version: | unspecified → 1.4.1 |
---|
follow-up: 4 comment:3 by , 17 years ago
Cc: | added |
---|---|
Component: | OGR_SRS → OGR_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 by , 17 years ago
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 by , 17 years ago
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
by , 17 years ago
Attachment: | ogrinfo.cs added |
---|
comment:6 by , 17 years ago
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 by , 17 years ago
Cc: | 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 by , 17 years ago
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 by , 17 years ago
tamas
As requested I have zipped up my app dir and posted it. It includes the VS2005 project and S57 files.
Regards & Thanks
Ben.
by , 17 years ago
Attachment: | gdal_gdal_csharp.2.dll added |
---|
by , 17 years ago
Attachment: | gdal_gdal_csharp.dll added |
---|
comment:10 by , 17 years ago
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 by , 17 years ago
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 by , 17 years ago
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 by , 17 years ago
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 by , 17 years ago
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 by , 17 years ago
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 by , 17 years ago
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 by , 17 years ago
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
follow-up: 19 comment:18 by , 17 years ago
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 by , 17 years ago
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 by , 17 years ago
Status: | new → assigned |
---|
comment:21 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.
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.