fix: remove incorrect noexcept from resource handle functions #1489
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Andy-Jost
added
bug
Contributor
Contributor
Author
|
/ok to test 4e70324 |
This comment has been minimized.
This comment has been minimized.
rwgk
approved these changes
Jan 14, 2026
Collaborator
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
LGTM
I did not scrutinize the case-by-case keep/remove decisions for the noexcepts. I assume an LLM was used to figure out those details.
Contributor
|
Should we add some cpp linting now that we have a significant amount of code to maintain there? Not in this PR of course :) |
Functions that allocate memory (via new, std::make_shared, or container operations) can throw std::bad_alloc. Marking them noexcept causes std::terminate on OOM instead of allowing graceful error handling. Changes: - Remove noexcept from 22 C++ functions that can throw - Keep noexcept on 5 functions that truly cannot throw: get_last_error, peek_last_error, clear_last_error, deallocation_stream, set_deallocation_stream - Update Cython .pxd/.pyx to use "nogil except+" for throwing functions - Wrap clear_mempool_peer_access in try-catch (std::vector can throw) - Wrap IPC cache deleter operations in try-catch (std::lock_guard can throw) This allows C++ exceptions to be translated to Python exceptions by Cython, enabling proper error handling instead of process termination. Based on feedback from Leo Fang, Lawrence Mitchell, and Vyas Ramasubramani.
Andy-Jost
force-pushed
the
fix-noexcept-resource-handles
branch
from
4e70324 to
2e1a8dc
Compare
Contributor
Author
yes! |
Contributor
Author
|
/ok to test 2e1a8dc |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
noexceptfrom C++ resource handle functions that can thrownogil except+for proper exception translationChanges
1. Remove incorrect
noexceptfrom throwing functionsFunctions that allocate memory (via
new,std::make_shared, or container operations) can throwstd::bad_alloc. Marking themnoexceptcausesstd::terminateon OOM instead of allowing graceful error handling.Remove
noexceptfrom 22 functions:create_*functions (context, stream, event, mempool, deviceptr)deviceptr_alloc*functionsget_primary_context,get_current_context,get_device_mempoolget_legacy_stream,get_per_thread_streamKeep
noexcepton 5 functions that truly cannot throw:get_last_error,peek_last_error,clear_last_errordeallocation_stream,set_deallocation_stream2. Update Cython declarations
Change from
noexcept nogiltonogil except+for all throwing functions, allowing Cython to translate C++ exceptions to Python exceptions.3. Fix exception-unsafe deleters
Custom
shared_ptrdeleters must not throw (undefined behavior per C++ standard). Two deleters had potential issues:clear_mempool_peer_access: Usesstd::vectorwhich can throwstd::bad_alloc. Wrapped in try-catch.std::lock_guardwhich can throwstd::system_error. Wrapped cache operations in try-catch.