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.