set_env/unset_env should release memory

[rblazek]

set_env/unset_env should release memory allocated for previously set value if a name is reused/deleted.

[glynn]

Replying to rblazek:

set_env/unset_env should release memory allocated for previously set value if a name is reused/deleted.

That would require G__getenv() to duplicate the value. Currently, it just returns the pointer. As the caller may hold onto the pointer indefinitely, the library cannot free it. Duplicating the value would require the caller to free it.

I'm not sure this is worth it. Do any GRASS modules call G_setenv() repeatedly?

[rblazek]

GRASS modules probably don't set variables repeatedly, but in GDAL, for example, if working with GRASS rasters from more mapsets/locations simultaneously, needs to reset mapset/location if masking is not disabled. It is using G_set_window() which checks for MASK.

Is there any other reasonable way how to reset environment when reading rasters from more mapsets/locations?

[glynn]

Replying to rblazek:

GRASS modules probably don't set variables repeatedly, but in GDAL, for example, if working with GRASS rasters from more mapsets/locations simultaneously, needs to reset mapset/location if masking is not disabled. It is using G_set_window() which checks for MASK.

Is there any other reasonable way how to reset environment when reading rasters from more mapsets/locations?

If you need to switch between two distinct environments (e.g. source and destination), there's G__create_alt_env() and G__switch_env().

If you need more than two environments, you're out of luck.

Changing this would mean that many library functions calling G_getenv() etc would need to free the memory when it's no longer needed, which means that they would need to either track when it's no longer needed or push this burden onto the caller.

In large software packages, tracking dynamically-allocated memory can be a major issue. Eliminating this issue is one of the main "selling points" of high-level languages with built-in garbage collection.

The GRASS philosophy of not worrying about memory leaks which will only occur a few times per process (for a normal GRASS module) saves us a lot of trouble.

[rblazek]

Replying to glynn:

Thanks for explanation. Two environments are not sufficient because GDAL/QGIS can open any number of maps from different locations but the idea of G__switch_env() sounds good. Do you think that a patch adding possibility to create more environments and switching between them using environment id/index would be accepted? I am thinking about an array of pointers to a structure keeping ENV *env and mapset_name (maybe more?).

[glynn]

Replying to rblazek:

Do you think that a patch adding possibility to create more environments and switching between them using environment id/index would be accepted?

That seems reasonable.

[neteler]

See also #864