FeatureIterator doesn't reset if the iterator isn't completely read

[nms-scribe]

When looping through a feature iterator, an early break can cause problems later in the program run, because reset_feature_reading is never called. The workaround is, to call reset_feature_reading after breaking early from such a loop, but failing to do this can cause errors that are difficult to diagnose.

I don't know how this would be fixed, but I think this should at least be documented under Layer::features().

The following is a test I built to reproduce the issue. It's a lot of code, but it takes a few operations before it gets into a situation which results in an error.

This test always fails on my machine, unless I uncomment the code where I've written "FIX IS HERE". Except for a few gdal errors (which are printed to stderr instead of returning results for some reason), the consequences don't occur until far later in the code where I've written "ERROR HAPPENS HERE.".

#[test]
#[should_panic(expected="create should not return an an error here, but it does for now: OgrError { err: 6, method_name: \"OGR_L_CreateFeature\" }")]
fn test_database_lock_issue() {
    use std::path::PathBuf;
    use gdal::Dataset;
    use gdal::GdalOpenFlags;
    use gdal::DriverManager;
    use gdal::LayerOptions;
    use gdal_sys::OGRwkbGeometryType;
    use gdal::vector::LayerAccess;
    use gdal::vector::Feature;
    use gdal::spatial_ref::SpatialRef;

    fn edit_dataset(test_file: PathBuf, finish_loop: bool) {
        let mut dataset = if (&test_file).exists() {
            Dataset::open_ex(&test_file, gdal::DatasetOptions { 
                open_flags: GdalOpenFlags::GDAL_OF_UPDATE, 
                ..Default::default()
            }).expect("open dataset")
        } else {
            let driver = DriverManager::get_driver_by_name("GPKG").expect("get driver");
            driver.create_vector_only(&test_file).expect("create dataset")
        };
    
        let mut transaction = dataset.start_transaction().expect("start transaction");
    
        const RIVERS: &str = "rivers";
        let mut rivers = transaction.create_layer(LayerOptions {
                    name: RIVERS,
                    ty: OGRwkbGeometryType::wkbNone,
                    srs: None,
                    options: Some(&["OVERWRITE=YES"])
                }).expect("create layer");
        rivers.create_defn_fields(&[]).expect("define fields");
    
        { // put in a block so we can borrow rivers as mutable again.
            let feature = Feature::new(&rivers.defn()).expect("new feature");
            feature.create(&rivers).expect("create feature");
        }
    
        // I think this is where the problem is...
        for _ in rivers.features() {
            // break early...
            if !finish_loop {
                break;
            }
        };
        //gdal::vector::LayerAccess::reset_feature_reading(&mut rivers); // FIX IS HERE!
    
    
        const COASTLINES: &str = "coastlines";
        let coastlines = transaction.create_layer(LayerOptions {
            name: COASTLINES,
            ty: OGRwkbGeometryType::wkbPolygon,
            srs: Some(&SpatialRef::from_epsg(4326).expect("srs")),
            options: Some(&["OVERWRITE=YES"])
        }).expect("create layer");
    
        coastlines.create_defn_fields(&[]).expect("defn_fields");
    
        { // in a block so transaction can be borrowed again.
            let feature = Feature::new(coastlines.defn()).expect("new feature");
            // ERROR HAPPENS HERE.
            feature.create(&coastlines).expect("create should not return an an error here, but it does for now");
        }
    
        transaction.commit().expect("commit");
    
        dataset.flush_cache().expect("flush");
    }
    

    let test_file: PathBuf = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("target").join("tmp").join("test_database_locked.gpkg");
    
    // delete the file if it exists so I can rerun the test.
    _ = std::fs::remove_file(test_file.clone()); // ignore error
    edit_dataset(test_file.clone(),false);
    // this one doesn't cause the error, so it's not because the database already exists...
    edit_dataset(test_file.clone(),true);
    // this one will error
    edit_dataset(test_file,false)


}

[lnicola]

Yeah, that's pretty bad, we should reset it in features or when the iterator gets dropped.

[lnicola]

I tried to look a little into this. We currently rewind (OGR_L_ResetReading) before the iteration. The docs also say that:

Structural changes in layers (field addition, deletion, ...) when a read is in progress may or may not be possible depending on drivers. If a transaction is committed/aborted, the current sequential reading may or may not be valid after that operation and a call to OGR_L_ResetReading() might be needed.

So the problem is not Feature::create, but rather adding the new layer before rewinding.

I think it would make slightly more sense to do it at the end, when the iterator is dropped, because it plays better with manual OGR_L_GetNextFeature calls that the user might make (I don't have a use case in mind though). So maybe we could try moving the OGR_L_ResetReading to drop.