apple / apple/foundationdb

Directory.move() modifies the database even when it errors

Open
#2,925 3 comments 0 reactions 1 assignee Claimed by @ajbeamon View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.