Directory.move() modifies the database even when it errors
- Dominant language
- C++
- Stars
- 16.7k
- Forks
- 1.6k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 126
Description
I spent the last day or so tracking down a behaviour issue in the new directory layer in the nodejs bindings that the bindingtester ran into. The issue is in `DirectoryLayer#move()`. If the directory passed to the function is invalid in any way, move still sets the version information in the database layer before throwing. From [directory_impl.py](https://github.com/apple/foundationdb/blob/867f734d8f3f7d6e512d42d30c530233c223f0d7/bindings/python/fdb/directory_impl.py#L326-L365):
```python
@_impl.transactional
def move(self, tr, old_path, new_path):
self._check_version(tr) # <-- writes version information to the database
# ...
if old_path == new_path[:len(old_path)]:
raise ValueError("The destination directory cannot be a subdirectory of the source directory.")
# ...
if not old_node.exists():
raise ValueError("The source directory does not exist.")
# etc more error checking before actually performing the move
tr[parent_node.subspace[self.SUBDIRS][new_path[-1]]] = self._node_subspace.unpack(old_node.subspace.key())[0]
self._remove_from_parent(tr, old_path)
```
In comparison, [create_or_open](https://github.com/apple/foundationdb/blob/867f734d8f3f7d6e512d42d30c530233c223f0d7/bindings/python/fdb/directory_impl.py#L233) tries to avoid this problem - it uses `self._check_version(tr, write_access=False)` to validate the version information, and only later calls `self._check_version(tr)` to set the version information if the call will attempt to create the directory.
Anyway, I don't know if this is a bug or not, or what the behaviour should be. I'm happy with "it doesn't matter much" but the bindingtester depends on this behaviour being consistent between all bindings and I'd like to get another opinion about the right approach here.
---
The error came from bindingtester at git tag 6.2.19 - ref 0a46c6276.
Command:
```
$ ./bindingtester.py nodejs --test-name directory --seed 733301871 --concurrency 1 --api-version 620 --num-ops 43 --compare
```
This test case calls `directory.move((), ())`, which raises an exception. But before that happens, the call to `move` sets the version info in the directory. Another concurrent write also sets the version information, and the result is a write conflict error. That error was missing in the node bindings.
Contributor guide
Assessment
This issue has not been assessed yet.