Repository navigation
Conversation
mdboom
added
bug
mdboom
changed the title
📝 Summary
Priority: ➖ Normal Change: Bug fix Merge Risk: 🔵 Low · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments: 1
cuda_core/tests/test_allocation_failure.py: 1
165-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Assert a failure inside default-stream binding.
The first
Buffer.from_handleattempt can raiseMemoryErrorwhile creating its pointer handle. That result alone satisfies this assertion, even if no attempt reaches the new cleanup inBuffer._init. Identify the allocation that fails duringset_deallocation_stream, or instrument the shim to confirm that call was reached, and assert that outcome separately.As per path instructions, check that “public behavior changes have corresponding tests.”
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
be0d2d91-33c8-454f-b0cd-38822486029d
📒 Files selected for processing: 9
cuda_core/cuda/core/_cpp/rt/api.hppcuda_core/cuda/core/_cpp/rt/internal.hppcuda_core/cuda/core/_cpp/rt/memory.cppcuda_core/cuda/core/_cpp/rt/stream.cppcuda_core/cuda/core/_memory/_buffer.pyxcuda_core/cuda/core/_rt.pxdcuda_core/cuda/core/_rt.pyxcuda_core/docs/source/release/1.3.0-notes.rstcuda_core/tests/test_allocation_failure.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments: 1
cuda_core/tests/test_allocation_failure.py: 1
98-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winsuggestion: Extend the allocation-failure harness to cover
make_deviceptr_box.
test_allocation_failure.pydoes not calldeviceptr_alloc_from_pool,deviceptr_alloc_async, ordeviceptr_import_ipc. Add one focused scenario that forcesnew DevicePtrBoxto throw after the CUDA allocation, then assertsMemoryError, a zero child exit status, and resource cleanup. This covers the shared cleanup branch without testing every allocation count.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
21d485aa-b15d-4e0e-a9c8-affbb174901e
📒 Files selected for processing: 1
cuda_core/cuda/core/_cpp/rt/memory.cpp
🚧 Files skipped from review as they are similar to previous changes: 1
- cuda_core/cuda/core/_cpp/rt/memory.cpp
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · important: Unmap the graphics resource when handle creation throws. · memory.cpp:345-349
cuda_core/cuda/core/_cpp/rt/memory.cpp:345-349
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winimportant: Unmap the graphics resource when handle creation throws.
cuGraphicsMapResourcessucceeds beforemake_deallocation_streamcan throwstd::bad_alloc. The unmap-owning handle is not constructed, and_mapped_bufferremains unset.GraphicsResource.close()therefore cannot unmap the resource. A latermap()call can fail because the resource is still mapped.Suggested fix
DeallocationStream ds; - if (!make_deallocation_stream(h_stream, ds)) { - return {}; + try { + if (!make_deallocation_stream(h_stream, ds)) { + return {}; + } + } catch (...) { + GILReleaseGuard gil; + CUgraphicsResource resource = as_cu(h_resource); + cleanup_in_context( + {}, "cuGraphicsUnmapResources", handle_bits(resource), + [&]() noexcept { + return DRIVER_CALL(cuGraphicsUnmapResources, + 1, &resource, as_cu(h_stream)); + }); + throw; }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
205354a6-7984-4b2a-8508-0b311fb6c6b3
📒 Files selected for processing: 1
cuda_core/tests/test_allocation_failure.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
This branch has not been deployed
1 participant
I have been working on adding
clang-tidytopre-commitforcuda-python. There is exactly one (potentially) real bug it found (the rest is just helping to make the code clearer to prevent /future/ bugs). So with that said, this is a theoretical bug, but something that has been observed in practice. We can decide this isn't worth the effort and just suppress theclang-tidyobservation about it instead.My agent's summary is below:
Fix:
std::bad_allocaborts the process inmake_deallocation_stream/set_deallocation_streamBranch:
handle-memory-exception(based onupstream/mainat e957c5c).Package:
cuda_core. Nothing here is committed.Summary
make_deallocation_stream()andset_deallocation_stream()in the C++ handlelayer (
cuda_core/cuda/core/_cpp/rt/) were declarednoexcept, but both canallocate host memory. When that allocation fails,
std::bad_allocescapes anoexceptfunction and the runtime callsstd::terminate, which aborts thewhole Python process (SIGABRT). It should raise
MemoryError.The fix removes
noexceptfrom both functions, declares the Cython binding ofset_deallocation_streamasexcept+(sostd::bad_allocbecomesMemoryError), and handles the one caller that needs cleanup before the errorpropagates. It is a robustness fix: it only matters when the host is out of
memory, but the failure mode (an unrecoverable abort) is exactly what the
project's own guidelines forbid.
How it was found
clang-tidy'sbugprone-unhandled-exception-at-newflagged the barenewin
make_deallocation_stream:The warning alone does not say why it matters, and it points at only one of the
two throwing sites. The rest of the analysis below was done by reading the code.
Why it is a bug
cuda_core/CLAUDE.md("Failure handling", "Never terminate the process") says:make_deallocation_stream(h, out) noexcept(stream.cpp) binds a deallocationstream to a stream handle. Real streams are copied unchanged, but a
default-stream token (
nullptr,CU_STREAM_LEGACY,CU_STREAM_PER_THREAD)with no embedded context takes this path:
get_current_context()is called. It is notnoexceptand allocates:it goes through
create_context_handle_ref, which builds astd::shared_ptr(a control-block allocation).new StreamBox{...}and thestd::shared_ptrthat owns it allocate.Either allocation can throw
std::bad_alloc. Because the enclosing function isnoexcept, the exception cannot propagate andstd::terminateruns.set_deallocation_stream(h, h_stream) noexcept(memory.cpp) calls it, so itinherited the problem. It is the C++ side of the public
Buffer.set_deallocation_stream(), and it is also called fromBuffer.from_handle()whenever the memory resource is device accessible.The other callers of
make_deallocation_stream(deviceptr_alloc_from_pool,deviceptr_alloc_async,deviceptr_create_mapped_graphics,deviceptr_import_ipc) are notnoexcept, and their Cython declarations arealready
except+, so they were already safe once the callee may throw.The fix
C++ (
cuda_core/cuda/core/_cpp/rt/):stream.cpp,internal.hpp:make_deallocation_streamis no longernoexcept. The comment documents that it may throwstd::bad_alloc.memory.cpp,api.hpp:set_deallocation_streamis no longernoexcept.The header comment now says it may throw
std::bad_alloc, matching howcreate_vmm_rangedocuments the same thing.Cython:
_rt.pyx,_rt.pxd:set_deallocation_streamis declaredexcept+ nogilinstead ofnoexcept nogil, like its siblingdeviceptr_import_ipc. Cython convertsstd::bad_allocintoMemoryError._memory/_buffer.pyx:_apply_deallocation_stream(used byBuffer.set_deallocation_stream)is declared
except *, so theMemoryErrorpropagates with no change.Buffer._init(used byBuffer.from_handle) already resetsself._h_ptrbefore raising on a failed status, because the
DevicePtrHandledestructorcalls
_mr_dealloc_callback, whoseexcept Exceptionhandler can clear anin-flight error indicator. The new
MemoryErrorpath does the same reset,inside an
except MemoryError:handler and then re-raises, so the samehazard cannot swallow the error.
Release note added to
cuda_core/docs/source/release/1.3.0-notes.rst.Alternatives considered
std::bad_allocin C++ and returnCUDA_ERROR_OUT_OF_MEMORY. Thiswould leave the Cython signature untouched, but it reports host exhaustion as
a device error, and it hides the real exception type.
MemoryErroris theaccurate Python exception and is what the sibling functions already raise.
noexceptand wrap the body intry/catch. This still needs somechannel to report the failure, so it ends up as one of the two options above.
Testing
New file
cuda_core/tests/test_allocation_failure.py. A tiny shared library(compiled with the system C++ compiler during the test) replaces
operator newso that the Nth C++ allocation on the arming thread throwsstd::bad_alloc. It is preloaded withLD_PRELOADinto a child process, soan abort shows up as a non-zero exit status instead of killing the test run.
Python allocates through its own allocators, so only C++ allocations are
affected. The tests are Linux-only and skip if no C++ compiler is available.
test_set_deallocation_stream_raises_memory_error_instead_of_terminating:fails the first C++ allocation inside
Buffer.set_deallocation_streamwith adefault-stream token and asserts a clean
MemoryError.test_buffer_from_handle_survives_allocation_failure: fails each of thefirst eight allocations on the way through
Buffer.from_handleand asserts thechild exits normally and at least once saw a
MemoryError.Results (GPU run on this machine):
test_allocation_failure.pyterminate called after throwing an instance of 'std::bad_alloc'I confirmed the "without" row by reverting only the source changes, rebuilding,
and running the new tests.
Wider run with the fix:
pytest cuda_core/tests --ignore=cuda_core/tests/cythongave 4081 passed, 201 skipped, 6 xfailed, 1 failed. The one failure,
test_program_cache.py::test_filestream_write_over_cap_skips_vanished_files, isunrelated and flaky: three consecutive runs of that single test gave pass, fail,
pass. The repo's pre-commit hooks pass on the changed files.
Limits and follow-ups
noexceptfunction bodyunder
_cpp/rt/fornew,make_shared, containers andstd::stringfoundno other direct allocation. Transitive cases (a
noexceptfunction calling athrowing function) are not caught by any stock clang-tidy check, so there may
be others. The fault-injection shim used here would find them if run across
the whole suite (fail the Nth allocation for many N).
throws (a possible leak under memory exhaustion) were not audited. This fix
does not change them.