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.
There was a problem hiding this comment.
Sorry, yes, fixed it now!
shoyer
left a comment
shoyer
left a comment
There was a problem hiding this comment.
Generally this looks great to me!
Please take a look at the failing tests, looks like there are a few details you missed
xarray/backends/file_manager.py
Outdated
| @@ -394,7 +399,11 @@ def close(self, needs_lock: bool = True) -> None: | |||
| del needs_lock # unused | |||
dschwoerer
force-pushed
the
netcdf4-locking-closing
branch
from
b35c80f to
d37c6ee
Compare
|
@shoyer The tests should be passing now, so hopefully this can be merged soon. Sorry for the delay! |
|
Looks like this one was just about ready and slipped through the cracks. I am going to update the branch to see if that'll get the tests passing. |
As @shoyer mentioned, it has no impact
|
This is great! @dschwoerer do you want to add a section to what's new? |
- Add missing bug fixes: pydata#11064, pydata#11026, pydata#11022, pydata#11005, pydata#10980, pydata#10788, pydata#11085 - Add missing documentation entries: pydata#10958, pydata#11094, pydata#11080 - Add missing performance entry: pydata#11105 - Add Internal Changes section with pydata#11108 - Fix rST roles: use :py:func:, :py:meth:, :py:class: consistently - Fix missing commas between :issue: and :pull: references - Fix typo in Dataset.identical entry - Fix wrong PR number for coord ordering (11098 -> 11091) - Add missing :pull: reference for arithmetic_compat (pydata#10943) Co-authored-by: Claude <noreply@anthropic.com>
4 participants
It did run the test 100 times without issues. Before that every ~1 out of 3 runs failed
whats-new.rstapi.rst