Need to reset feature iterators on early break

[nms-scribe]

Revisit the target.reedit problem in big_bang, see if I can get an MRE that causes the problem it was intended to patch, and track down the real problem.

[nms-scribe]

I fixed this, but I'm not going to close this because I did pinpoint it to a bug in the gdal crate, I still need to create an MRE to prove it to them, though. This issue remains open as a reminder to me to do that.

These are my comments from the original usage of reedit in the BigBang::run_default, which appeared before the call to GenWater::run_default

        // FUTURE: If I don't do the next line, I get an error in the next command parts from SQLite that 'coastlines' table is locked. If I remove the 
        // algorithm immediately following, I get the same error for a different table instead. The previous algorithms don't even touch those items, and if the
        // file already exists (which it did when I was running this error), 'create_or_edit' is the same as 'edit', so there isn't some
        // special case create locking going on.
        // - Maybe some future version of gdal or the gdal crate will fix this. If it does it's a simple matter of removing this line.
        // - I do not know if there's another way to fix it, but this was my first thought, and it works, and I don't want to go any further because I'm being triggered with memories of Windows 2000 DLL and ActiveX code integrations where this sort of thing was the only answer. Shudder.
        /* The specific error messages:
            ERROR 1: sqlite3_exec(DROP TABLE "rtree_coastlines_geom") failed: database table is locked
            ERROR 1: sqlite3_exec(DROP TABLE "coastlines") failed: database table is locked
            ERROR 1: sqlite3_exec(CREATE TABLE "coastlines" ( "fid" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "geom" POLYGON)) failed: table "coastlines" already exists
            ERROR 1: sqlite3_exec(CREATE VIRTUAL TABLE "rtree_coastlines_geom" USING rtree(id, minx, maxx, miny, maxy)) failed: table "rtree_coastlines_geom" already exists
            gdal: OGR method 'OGR_L_CreateFeature' returned error: '6'

         */
        let mut target = target.reedit()?;

The issue turned out to be caused in PropertyLayer::get_property. This was the original code for that:

    pub(crate) fn get_property(&mut self, name: &str) -> Result<String,CommandError> {
        for feature in TypedFeatureIterator::<PropertySchema,PropertyFeature>::from(self.layer.features()) {
            if feature.name()? == name {
                return feature.value()
            }
        }
        Err(CommandError::PropertyNotSet(name.to_owned()))

    }

After I added references to a new property (world-shape), the error started appearing later in the algorithm. An additional call to reedit was placed before this call, but then the error appeared even later, at places that I could not fix without moving the reedit calls into the algorithms themselves.

Since it was obviously related to the new property, and in those cases only occurred when retrieving that property value, I looked through that process to see what I could fix. It occurred to me that the feature iterator might not be "closing" itself off because I did an early return.

Changing the get_property method to the following removed the error in that case, and after I removed all calls to reedit, it was no longer occurring.

    pub(crate) fn get_property(&mut self, name: &str) -> Result<String,CommandError> {
        let mut result = None;
        for feature in TypedFeatureIterator::<PropertySchema,PropertyFeature>::from(self.layer.features()) {
            if feature.name()? == name {
                result = Some(feature.value()?)
            }
        }
        if let Some(result) = result {
            Ok(result)
        } else {
            Err(CommandError::PropertyNotSet(name.to_owned()))
        }
    }

So, it's clear that the layer.features() operation somehow leaves a lock on database in FeatureIterator unless you iterate through the features completely. In reviewing the gdal crate code, I can't find anything they're doing to clean it out after the last iteration (which I would be skipping with an early return), so it's obviously something in gdal not cleaning up a lock unless you get to the last feature. Perhaps they're supposed to call a different function on an early end to the iteration.

I need to do more research on this, and reproduce the issue for them to worry about, but I don't have time right now.

[nms-scribe]

The fix is to change the get_property to call self.layer.reset_feature_reading() after iterating. I don't know whether this should be reported to gdal crate people as their error? Is it a documentation error, at least? It is documented for the gdal library (or at least it's inner functions are: https://gdal.org/api/vector_c_api.html#_CPPv420OGR_L_GetNextFeature9OGRLayerH).

[nms-scribe]

I've opened up an issue for this at the gdal crate (georust/gdal#507 (comment)). I can revisit this after that is dealt with.