Skip to content

fix(python): add collection close method#567

Open
sjh9714 wants to merge 2 commits into
alibaba:mainfrom
sjh9714:fix-python-collection-close
Open

fix(python): add collection close method#567
sjh9714 wants to merge 2 commits into
alibaba:mainfrom
sjh9714:fix-python-collection-close

Conversation

@sjh9714

@sjh9714 sjh9714 commented Jul 8, 2026

Copy link
Copy Markdown

Summary

Closes #563.

Adds Collection.close() to release the Python wrapper's underlying _Collection handle, so callers can explicitly drop native resources before reopening or removing a collection path. The concurrent-open test now records the opened path before closing the worker handle.

Note: I used Codex to help prepare this change, reviewed the final diff, and ran the listed checks locally.

Testing

  • git submodule update --init --recursive --depth=1
  • python3 -m venv .venv
  • .venv/bin/python -m pip install --upgrade pip setuptools wheel
  • .venv/bin/python -m pip install -e ".[test]"
  • .venv/bin/python -m pytest python/tests/detail/test_collection_open.py::TestCollectionOpen::test_close_releases_collection_for_reopen -q
  • .venv/bin/python -m pytest python/tests/detail/test_collection_open.py -q
  • .venv/bin/python -m compileall -q python/zvec/model/collection.py python/tests/detail/test_collection_open.py
  • .venv/bin/python -m pip install "ruff>=0.4"
  • .venv/bin/python -m ruff check python/zvec/model/collection.py python/tests/detail/test_collection_open.py
  • git diff --check

@sjh9714
sjh9714 requested a review from Cuiyus as a code owner July 8, 2026 15:32
@CLAassistant

CLAassistant commented Jul 8, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Cuiyus

Cuiyus commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Thanks for putting together this PR.
Could we expose an explicit native Collection::Close() instead of implementing close() by only dropping the Python reference?
self._obj = None merely decrements the refcount of the pybind holder. It triggers CollectionImpl destruction only when there are no other references, so an in-flight operation, a shallow copy, or a retained coll._obj reference may keep the native collection and its file lock alive after close() returns. This is effectively the same behavior already available through del coll when coll is the last reference.

Close() flushes and releases segments, managers, and the collection file
lock even while other references to the handle remain alive. Post-close
operations are rejected with "collection is already closed" instead of
touching released state, and closing twice is a no-op. Python
Collection.close() now calls the native Close() before dropping its
references.
@sjh9714
sjh9714 requested review from chinaux and zhourrr as code owners July 21, 2026 22:05
@sjh9714

sjh9714 commented Jul 21, 2026

Copy link
Copy Markdown
Author

Thanks for the review — agreed. Dropping the Python reference only decrements a refcount, so it gives no guarantee when other references, shallow copies, or in-flight operations keep the object alive.

I've reworked the PR to expose an explicit native Collection::Close():

  • Collection::Close() is now a public virtual on the interface, and CollectionImpl::Close() reuses the existing close_unsafe() teardown (flush for read-write collections, release segments/managers, close the lock file). It takes the exclusive schema_handle_mtx_, so it serializes with in-flight operations, and closing twice is a no-op.
  • A closed_ flag is checked alongside the existing destroyed_ guards, so operations on a closed collection return InvalidArgument("collection is already closed.") instead of touching released state through a stale handle. Destroy() after Close() is also rejected — the intended flow for a closed path is to reopen it (or remove the directory directly, which is what [Bug]: Python Collection 缺少 close() 方法,导致 Windows 文件锁无法释放 #563 needs on Windows).
  • The pybind layer exposes Close() (GIL released, same shape as the existing Destroy binding), and Python Collection.close() now calls it before dropping its references.
  • Regression tests: close() releases the file lock even while another reference to the native handle is still alive (reopen succeeds), post-close operations through a stale handle raise ValueError instead of crashing, and double close is a no-op.

While rerunning the suite I also noticed test_collection_create_and_open.py::test_open_concurrent_same_path kept the closed wrapper object and read .path on it afterwards; I gave it the same treatment as its twin in test_collection_open.py (record the path string before closing).

One note on scope: the C API's zvec_collection_close still just deletes its wrapper reference. Happy to wire it to the native Close() as a follow-up if you'd like.

Testing: pip install -e ".[dev]" (full native rebuild) · pytest python/tests/ — 1107 passed, 70 skipped (platform-only skips) · ruff check / ruff format --check on the changed Python files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Python Collection 缺少 close() 方法,导致 Windows 文件锁无法释放

3 participants