Ensure netcdf4 is locked while closing #10788
Conversation
|
Note that this only a partial fix, for the |
xarray/backends/file_manager.py
Outdated
| # Check for string so we do not have to import it | ||
| if str(type(file)) == "<class 'netCDF4._netCDF4.Dataset'>": | ||
| with NETCDF4_PYTHON_LOCK: | ||
| file.close() | ||
| return |
There was a problem hiding this comment.
Can we add a parameter to FileManager (and also probably PickleableFileManager) in a way that allows this logic to stay only in the backend class?
I'm imagining adding a parameter closer, which is used to close files like closer(file).
There was a problem hiding this comment.
Could also call this close but elsewhere in file_manager this is used for functions with a signature like close(), which could be a bit ambiguous.
There was a problem hiding this comment.
I noticed when can just store the lock in the FileManager, then that does not require significant changes, including making it self-contained, without needing a closer function
dschwoerer
force-pushed
the
netcdf4-locking-closing
branch
from
edcbfc2 to
e7c84a7
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| self._args = args | ||
| self._mode = "a" if mode == "w" else mode | ||
| self._kwargs = kwargs | ||
| self._lock = lock |
There was a problem hiding this comment.
The rest of PickleableFileManager needs updates to use lock, at least in the close and __getstate__/__setstate__ methods.
There was a problem hiding this comment.
I am not sure about __getstate__ and __setstate__ - for __getitem__ and __setitem__ the netCDF4 backend is doing the locking.
Are you sure we need to lock for them?
If we would switch to RLocks, we would need to be less strict about these checks ...
I will change it to use a lock in close 👍
There was a problem hiding this comment.
__getstate__/__setstate__ are methods for pickling the file manager object. We definitely need to add lock there, otherwise the lock will be dropped when the file manager is pickled/unpickled.
It did run the test 100 times without issues. Before that every ~1 out of 3 runs failed
whats-new.rstapi.rst