cuda.core: reject operations on closed resource objects - #2635
Conversation
Andy-Jost
added
bug
|
| q_bc.put(buffer) | ||
| buffer.close() | ||
|
|
||
| # Wait for C to receive before exiting. | ||
| # Queue serialization runs in a feeder thread. Keep the buffer open | ||
| # until C has received it and the parent releases this process. | ||
| event_b.wait(timeout=CHILD_TIMEOUT_SEC) | ||
| buffer.close() |
There was a problem hiding this comment.
This fixes a latent bug. multiprocessing.Queue.put requires its argument to remain valid until received.
0539baa to
31f74c3
Compare
31f74c3 to
84db682
Compare
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
General comment -- move the helper functions to inline implementations in the .pxd. Then I see a mix of calling these helper functions and doing an if closed: raise(...). Is there a reason for that difference? If not, maybe consistently use the helper functions?
| return tuple(out) | ||
|
|
||
|
|
||
| cdef int Buffer_check_open(Buffer self) except -1: |
There was a problem hiding this comment.
Since it's called on basically every path, inlining should help. Since this is used from other modules, move the implementation to the .pxd and add the inline keyword.
| ) except? -1 | ||
|
|
||
| cdef int MP_raise_release_threshold(_MemPool self) except? -1 | ||
| cdef int MP_check_open(_MemPool self) except -1 |
There was a problem hiding this comment.
Move the implementation here and inline?
|
|
||
| def _get_int_attr(buf: Buffer, attribute: Any) -> int: | ||
| if buf.is_closed: | ||
| raise RuntimeError("Buffer has been closed") |
There was a problem hiding this comment.
I don't think the Cython function can be called from a pure Python module.
There was a problem hiding this comment.
Ah. I missed that this was a .py file. Makes sense.
| cdef int MP_check_open(_MemPool self) except -1: | ||
| if not self._h_pool: | ||
| raise RuntimeError(f"{self.__class__.__name__} has been closed") | ||
| return 0 |
Generally agree. The latest change consolidates open-state checks in inline functions. Shared Cython checks live in Details:
|
|
Python's precedent is We don't need to follow it, of course. |
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
Looks much better from the human-review standpoint.
Claude flagged a few things that seem like legitimate issues, but I don't have the full context to evaluate them.
| reference and allows the Python owner to be GC'd. | ||
| """ | ||
| if self._h_stream and Stream_is_default_token(self): | ||
| return |
There was a problem hiding this comment.
I think this one is legit, but I don't fully understand how these special Stream singletons work.
From Claude:
Stream.close() early-returns for any stream whose raw handle equals CU_STREAM_LEGACY/CU_STREAM_PER_THREAD, because Stream_is_default_token() compares raw handle values, not identity against the two default-stream singletons. Stream.from_handle(1) / Stream.from_handle(2) — legitimate user-created borrowed wrappers — become permanently un-closeable: the owner reference is never released and is_closed stays False forever, contradicting the method's own docstring. Fix: guard on self is LEGACY_DEFAULT_STREAM or self is PER_THREAD_DEFAULT_STREAM, not on handle value. The new test (test_raw_null_stream_is_live_until_closed) uses from_handle(0), which happens to sidestep this exact case.
There was a problem hiding this comment.
In thinking about this, I realized it’s probably better to remove the exception that prevents closing the default stream singletons. LEGACY_DEFAULT_STREAM and PER_THREAD_DEFAULT_STREAM are process-wide, so closing them may break the program, but they are still ordinary Stream objects. There is no justification for adding special semantics just because they happen to be global.
This would be like preventing users from closing sys.stdout. Going down that path is what led to this subtle problem in the first place. It’s better to let the global remain an ordinary object, even if it can be misused.
To clarify, LEGACY_DEFAULT_STREAM.close() only detaches the Python object from its handle. It does not instruct CUDA to destroy the default stream. This exception only protected the Python layer.
| asynchronously. Must be passed explicitly; pass | ||
| ``device.default_stream`` to use the default stream. | ||
| """ | ||
| cdef Stream s = Stream_accept(stream) |
There was a problem hiding this comment.
I think whether Claude is right about this depends on whether double-calling cuMemFreeAsync is an error.
From Claude:
_MemPool.deallocate() (backing DeviceMemoryResource/PinnedMemoryResource/ManagedMemoryResource) never got an MP_check_open(self) call, unlike every sibling entry point in the same file (allocate, attributes, MP_raise_release_threshold, __reduce__, peer_accessible_by). After mr.close(), calling mr.deallocate(ptr, size, stream=...) sails straight through to cuMemFreeAsync against a destroyed pool — exactly the failure class this PR is meant to close everywhere else.
There was a problem hiding this comment.
_MemPool.close() releases the Python object's memory pool handle but does not necessarily destroy the pool. Device pointer handles structurally embed an independent reference that ensures the pool remains live. I think the best option in this case is to leave deallocate unguarded and interpret close to mean future allocations are disallowed. Frees performed against a closed pool would remain valid. Otherwise, closing a pool would mean leaking any outstanding allocations that used Buffer.from_handle(..., mr=pool).
I think a case could be made that pools ought not be closable. In a language like Python, explicit close usually doesn't make sense. Exceptions are made when resources need to be relinquished accurately, though. I'm not sure that applies to CUDA memory pools.
There was a problem hiding this comment.
I updated the _MemPool.close docstring to clarify this.
| # Unpickling performs a live CUDA IPC import from descriptor bytes in the | ||
| # pickle stream. Only deserialize Buffers from a trusted principal. | ||
| # Must not serialize the parent's stream! | ||
| Buffer_check_open(self) |
There was a problem hiding this comment.
This one may or may not be legit. From Claude:
Adding Buffer_check_open(self) to __reduce__ breaks the queue.put(buffer); buffer.close() pattern: multiprocessing.Queue.put() serializes on a background feeder thread, so a race lets close() win and __reduce__ raises inside the feeder thread, where multiprocessing logs-and-discards it — the consumer just hangs with no error at the put() call site. The PR's own test (test_send_buffers.py:134) had to be reordered to work around this. This needs to be called out in the release notes (currently only mention rejecting closed resources, not this pickling/queue-handoff hazard).
There was a problem hiding this comment.
The pattern queue.put(buffer); buffer.close() was a latent race in the test that I discovered incidentally. That may explain why the test was marked flaky.
Because mp.Queue sends objects asynchronously, subsequent mutations (including close) must be ordered after the object is sent. I think the right thing in this case it to fix the test without publishing a release note.
| cdef inline void check_owner_mutable(self) except *: | ||
| if as_cu(self._h_graph) == NULL: | ||
| raise RuntimeError("GraphDefinition is no longer valid") | ||
| if as_cu(self._h_node) == NULL: |
There was a problem hiding this comment.
From Claude:
check_owner_mutable() treats any NULL _h_node as "destroyed," but GraphDefinition._entry (the virtual entry node) legitimately has _h_node == NULL by design — GN_check_valid correctly exempts it via _is_entry, but _AdjacencySetCore never captures that flag for the owner node. So graph_def._entry.succ.add(node) incorrectly raises "GraphNode has been destroyed" for a node that's actually valid. Exposure is limited (private _entry, not routed through this path by the public API today), but it's a real inconsistency in the exact contract this PR establishes, and untested.
There was a problem hiding this comment.
I believe this is unreachable and should be ignored. The virtual entry node is private and requesting its successors (or predecessors) is meaningless. Since there's no valid path to something like graph_def._entry.succ we should assume it is not used and avoid an unnecessary check.
| return self._h_fd.get() == NULL or as_intptr(self._h_fd) < 0 | ||
|
|
||
| def __int__(self) -> int: | ||
| if not self._h_fd or as_intptr(self._h_fd) < 0: |
There was a problem hiding this comment.
Minor nit:
| if not self._h_fd or as_intptr(self._h_fd) < 0: | |
| if not self.is_closed: |
There was a problem hiding this comment.
I noticed the negative-value check was superfluous, so I simply inlined the null-check in the few places where it was needed.
| @property | ||
| def is_closed(self) -> bool: | ||
| """Whether this allocation handle has been closed.""" | ||
| return self._h_fd.get() == NULL or as_intptr(self._h_fd) < 0 |
There was a problem hiding this comment.
Minor DRY nit:
| return self._h_fd.get() == NULL or as_intptr(self._h_fd) < 0 | |
| return IPCAllocationHandle_check_open(self) |
There was a problem hiding this comment.
Yeah, it's annoying to have two levels of inline helper for the "is it open" check and "raise if not open" check. I don't think this suggestion can be applied because it would raise. Fortunately, I was able to simplify it by removing the negative-value check.
| (<_AdjacencySetCore>self._core).check_owner_mutable() | ||
| if not isinstance(value, GraphNode): | ||
| return | ||
| (<_AdjacencySetCore>self._core).check_mutation(value) |
There was a problem hiding this comment.
Claude points out that this causes a diversion from the Python MutableSet.discard convention -- that an invalid value should just return and never raise. Therefore this check maybe belongs right before the remove_edge call.
cuda.core consistently names Boolean properties as |
Add consistent liveness checks so closed handles cannot reach CUDA as valid resources, including graph and cross-object operations.
The allocation-handle constructor is intentionally unsupported on Windows, so limit its close-state test to supported platforms.
Replace lifecycle-dependent truthiness with explicit is_closed and is_valid properties while preserving the historical truth value of cuda.core objects.
Centralize open and valid state checks so hot Cython call paths use one consistent implementation.
Expect the shared Context checker message so the green-context test matches the standardized validation path.
Keep generated type information aligned with the rebased lifecycle APIs.
868142e to
34b1cd1
Compare
34b1cd1 to
f8eae47
Compare
3 participants
Description
closes #2627
Add a consistent closed-state contract across
cuda.coreresource objects so released native handles are rejected before reaching CUDA. Closeable resources now exposeis_closed, whileGraphDefinitionandGraphNodeexposeis_validfor graph-lifetime invalidation. This change also adds tests ensuringbool(obj)returns true for closed and invalid objects, to retain backwards compatibility.Active methods validate their own state and accepted resource arguments.
Stream_accept()now rejects closed streams and graph builders, which also makesBuffer.set_deallocation_stream()andBuffer.close(stream=...)reject a closed stream without replacing the buffer's saved deallocation recipe. Closing CUDA default-stream tokens remains a no-op.The same validation covers events, buffers, memory pools, IPC handles, contexts, compiler resources, graphs, arrays, textures, surfaces, and graphics resources. Tests cover named lifecycle state, backward-compatible truthiness, idempotent cleanup, safe inspection, cross-object validation, graph invalidation, and deallocation-stream failure atomicity.
Checklist