Skip to content
29 changes: 24 additions & 5 deletions xarray/backends/file_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,10 @@ def close(self, needs_lock: bool = True) -> None:
with self._optional_lock(needs_lock):
default = None
file = self._cache.pop(self._key, default)
if file is not None:
if needs_lock and self._lock:
with self._lock:
file.close()
else:
file.close()

def __del__(self) -> None:
Expand Down Expand Up @@ -355,13 +358,15 @@ def __init__(
opener: Callable[..., T_File],
*args: Any,
mode: Any = _OMIT_MODE,
lock: Lock | None | Literal[False] = None,
kwargs: Mapping[str, Any] | None = None,
):
kwargs = {} if kwargs is None else dict(kwargs)
self._opener = opener
self._args = args
self._mode = "a" if mode == "w" else mode
self._kwargs = kwargs
self._lock = lock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of PickleableFileManager needs updates to use lock, at least in the close and __getstate__/__setstate__ methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__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.


# Note: No need for locking with PickleableFileManager, because all
# opening of files happens in the constructor.
Expand Down Expand Up @@ -394,7 +399,11 @@ def close(self, needs_lock: bool = True) -> None:
del needs_lock # unused
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line needs to be removed

if not self._closed:
file = self._get_unclosed_file()
file.close()
if needs_lock and self._lock:
with self._lock:
file.close()
else:
file.close()
self._file = None
# Remove all references to opener arguments, so they can be garbage
# collected.
Expand Down Expand Up @@ -448,9 +457,16 @@ def _remove_del_methods():
class DummyFileManager(FileManager[T_File]):
"""FileManager that simply wraps an open file in the FileManager interface."""

def __init__(self, value: T_File, *, close: Callable[[], None] | None = None):
def __init__(
self,
value: T_File,
*,
close: Callable[[], None] | None = None,
lock: Lock | None | Literal[False] = None,
):
if close is None:
close = value.close
self._lock = lock
self._value = value
self._close = close

Expand All @@ -464,5 +480,8 @@ def acquire_context(self, needs_lock: bool = True) -> Iterator[T_File]:
yield self._value

def close(self, needs_lock: bool = True) -> None:
del needs_lock # unused
self._close()
if needs_lock and self._lock:
with self._lock:
self._close()
else:
self._close()
12 changes: 8 additions & 4 deletions xarray/backends/netCDF4_.py
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ def __init__(
"argument is provided"
)
root = manager
manager = DummyFileManager(root)
manager = DummyFileManager(root, lock=NETCDF4_PYTHON_LOCK)

self._manager = manager
self._group = group
Expand Down Expand Up @@ -509,17 +509,21 @@ def open(
"<xarray-in-memory-write>", mode=mode, memory=memory, **kwargs
)
close = _CloseWithCopy(filename, nc4_dataset)
manager = DummyFileManager(nc4_dataset, close=close)
manager = DummyFileManager(nc4_dataset, close=close, lock=lock)

elif isinstance(filename, bytes | memoryview):
assert mode == "r"
kwargs["memory"] = filename
manager = PickleableFileManager(
netCDF4.Dataset, "<xarray-in-memory-read>", mode=mode, kwargs=kwargs
netCDF4.Dataset,
"<xarray-in-memory-read>",
mode=mode,
kwargs=kwargs,
lock=lock,
)
else:
manager = CachingFileManager(
netCDF4.Dataset, filename, mode=mode, kwargs=kwargs
netCDF4.Dataset, filename, mode=mode, kwargs=kwargs, lock=lock
)
return cls(manager, group=group, mode=mode, lock=lock, autoclose=autoclose)

Expand Down
Loading