Better defined and tested concurrent Envs

[sgillies]

Includes the work in PR #993.

Resolves #996.

[sgillies]

@geowurster I'm continuing to use CPLGetConfigOption in this branch because 1) the thread local version doesn't exist in GDAL 1.11 and 2) I don't understand the difference between CPLGetConfigOption and CPLGetThreadLocalConfigOption in GDAL 2.1. Each get a pointer to thread-local config options, right? The former uses a mutex (in a way I don't understand) and falls back on environment variables, the latter does not. @rouault can you explain what I'm missing and whether I'm going to experience trouble by not using CPLGetThreadLocalConfigOption?

More rigorous tests of the environments are forthcoming.

[rouault]

CPLGetConfigOption() will return the value of the config option, be it either defined through environement variable, CPLSetConfigOption() or CPLSetThreadLocalConfigOption() (from the same thread).

CPLGetThreadLocalConfigOption() will return the value of the config option, but only if it has been set with CPLSetThreadLocalConfigOption()

[geowurster]

@sgillies How do you feel about just issuing a warning when using Env() inside a thread on GDAL<2.0? Seems like we can use a shim and use CPL*ConfigOption() when CPL*ThreadLocalConfigOption() is not available.

Unless the user wants a different configuration per thread this should work on every GDAL version, although I think it requires a change in @ensure_env. Right now @ensure_env always creates a new environment, but if one exists it shouldn't create one.

from concurrent.futures import ThreadPoolExecutor

import rasterio as rio

def _process(path):
    with rio.open(path) as src:
        pass

with rio.Env(), ThreadPoolExecutor(4) as pool:
    for res in pool.map(_process, ['tests/data/RGB.byte.tif']):
        pass

[sgillies]

@geowurster I've made the change in ensure_env but I think that we can avoid the use of a shim. After more thought, I believe it's less surprising if child threads inherit the main thread's configuration and use CPLGetConfigOption.

Got a few minutes for review @geowurster?

[geowurster]

@sgillies sorry, didn't realize you were done. I should have some time tomorrow to review.

[geowurster]

I think being forced to wrap threads in a rasterio.Env() might be surprising for some. What do you think about something like this:

class GDALEnv):
    
    def __init__(self):
        self._have_registered_drivers = False

    def start(self):
        # The outer if statement prevents each thread from acquiring a lock when the environment starts, and the inner avoids a potential race condition.
         if not self._have_registered_drivers:
            with threading.Lock():
                if not self._have_registered_drivers:
                    GDALAllRegister()

This allows the first thread to quietly register drivers while still allowing the thread to inherit from a parent rasterio.Env() if needed, at the cost of some edge cases and potential surprises for those with passing familiarity of GDAL's internals.

I'm starting to think that the rasterio.Env() should be completely invisible by default but easy to manage for those with more advanced use cases, and I'm not entirely sure that wanting to just work with different files in different threads completely fits that advanced use case, especially since it doesn't necessarily require any modifications to the GDAL environment.

[geowurster]

I think its worth testing with a multiprocessing.Process() as well as doing a small I/O call to ensure that drivers are actually registered.

[sgillies]

@geowurster Agreed. I got it in the next two commits: thread and process pool executors, with and without an Env() in the main thread.

[sgillies]

Discovered options needed to be a thread local as well. I'm removing the "WIP" from the title.

[geowurster]

@sgillies Will take a look later today.

[geowurster]

This looks good! 🎉

I suspect the code coverage drop is due to some tests now running in threads and processes.

I made a few inline comments but none are blockers. Two more small requests:

1. Some tests introduced in this PR depend on concurrent.futures, which is not part of the standard library in Python 2, but has been backported as the futures package. We only get it because boto -> s3transfer -> futures. It's probably a good idea to add this to the tests install extras for Python 2.
2. Add tests/data/white-gemini-iv.zip to .gitignore

[geowurster]

No longer needed.

[geowurster]

Might not hurt to add "unless the option is set to a new value inside the thread"

[geowurster]

Could you move this long comment closer to _discovered_options, maybe just as a docstring on ThreadEnv()? It may provide those dealing with both osgeo and Rasterio with some important background info.

[geowurster]

This also closes #986