Skip to content

Add (failing) tests demonstrating that errors in Buffer.close are not raised #1117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add (failing) tests demonstrating that errors in Buffer.close are not raised #1117

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c92fb6c
Add failing tests demonstrating that errors in Buffer.close (for exam…
Andy-Jost Oct 9, 2025
751d9eb
Merge branch 'main' into ipc_suppressed_errors
Andy-Jost Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion cuda_core/tests/memory_ipc/test_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
import pickle
import re

import pytest
from cuda.core.experimental import Buffer, Device, DeviceMemoryResource, DeviceMemoryResourceOptions
from cuda.core.experimental._utils.cuda_utils import CUDAError
from cuda.core.experimental._utils.cuda_utils import CUDAError, driver

from cuda_python_test_helpers import supports_ipc_mempool

Expand Down Expand Up @@ -147,3 +148,66 @@ def CHILD_ACTION(self, queue):
def ASSERT(self, exc_type, exc_msg):
assert exc_type is RuntimeError
assert re.match(r"Memory resource [a-z0-9-]+ was not found", exc_msg)


def test_error_in_close_memory_resource(ipc_memory_resource):
"""Test that errors when closing a memory resource are raised."""
mr = ipc_memory_resource
driver.cuMemPoolDestroy(mr.handle)
with pytest.raises(CUDAError, match=".*CUDA_ERROR_INVALID_VALUE.*"):
mr.close()
Comment on lines +153 to +158
Copy link
Member

@leofang leofang Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is illegal and I disagree we need to test this. This is Python and we can't possibly guard against all kinds of bizarre ways of trying to mutate the state of our Python objects behind our back. In particular, as noted in both #1074 (comment) and offline discussion, errors like CUDA_ERROR_INVALID_VALUE are due to multiple frees. I thought we've moved on?

This test is just another instance of the same class of errors: We free the handle of an object through a direct C API call, bypassing our safeguard mechanism (under the hood we do check if the handle is already null before freeing, and then after free we set the handle to null to avoid double free), so our destructor kicks in, either through an explicit close() call or implicitly when going out of scope, and causes another free.

Copy link
Contributor

@cpcloud cpcloud Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this test is asserting that different levels of APIs offer the same guarantees, which would make developing layers of APIs really really difficult.

It seems roughly analogous to calling into the Python C API through ctypes, and expecting Python to somehow know you didn't mean to cause a segmentation violation:

❯ python3.13 -q
>>> x = 1
>>> import ctypes
>>> ctypes.pythonapi.Py_DecRef(x)
zsh: segmentation fault (core dumped)  python3.13 -q

Copy link
Contributor

@cpcloud cpcloud Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess there's probably another way to write this test such that the behavior is buggy without crossing into the C abyss of naked bindings.

Would it be enough to just call close() twice? That seems like something we should perhaps be robust to if we're not already:

❯ python -q
>>> f = open('/tmp/x', 'w')
>>> f.close()
>>> f.close()

Copy link
Contributor Author

@Andy-Jost Andy-Jost Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a misunderstanding. The point of this test is to check that errors occurring in close are raised rather than suppressed. The exact mechanism I chose to get the driver to generate an error is beside the point, If anyone sees a simpler mechanism, please point it out.

I ran into this issue (close not raising errors) when working with the driver bug 5570902. I don't want to rely on that behavior for the test because if the driver team ever fixes the bug, the test would then break.

Let's not confuse the issue: we are not testing our robustness in the face of nonsense behavior, i.e., someone stomping around in the driver API directly. This test just does something otherwise dumb and unsupported to trigger an error, which is totally reasonable for this test.

@cpcloud our code already tolerates double-calls to close and simply ignores the second call without error.

Copy link
Member

@leofang leofang Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no misunderstanding. Undefined behavior should not be tested. It is just confusing and sends a wrong signal.

re: on allowing the behavior that calling .close() is allowed to raise while invoking the destructor is not, I did raise a discussion with @pciolkosz on the Tuesday meeting. It is indeed nice to have, but it is unclear semantically what we can do about the exception, and technically it does not seem possible to implement in both C++ and Python (buffer.destroy(stream) from cccl-runtime is also noexcept). I suggest we table this discussion for later, and close this PR.



@pytest.mark.xfail(reason="Issue #1118", strict=True)
@pytest.mark.filterwarnings("ignore") # Test incorrectly warns
def test_error_in_close_buffer(ipc_memory_resource):
"""Test that errors when closing a memory buffer are raised."""
mr = ipc_memory_resource
buffer = mr.allocate(NBYTES)
driver.cuMemFree(buffer.handle)
with pytest.raises(CUDAError, match=".*CUDA_ERROR_INVALID_VALUE.*"):
buffer.close()
Comment on lines +167 to +169
Copy link
Member

@leofang leofang Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto



@pytest.mark.xfail(reason="Issue #1118", strict=True)
@pytest.mark.filterwarnings("ignore") # Test incorrectly warns
class TestErrorInCloseImportedMemoryResource(ChildErrorHarness):
"""Test that errors when closing an imported memory resource are raised."""

def PARENT_ACTION(self, queue):
pass

def CHILD_ACTION(self, queue):
try:
driver.cuMemPoolDestroy(self.mr.handle)
except Exception: # noqa: S110
pass
else:
self.mr.close()
Comment on lines +181 to +186
Copy link
Member

@leofang leofang Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


def ASSERT(self, exc_type, exc_msg):
assert exc_type is CUDAError
assert re.match(r"CUDA_ERROR_INVALID_VALUE", exc_msg)


@pytest.mark.xfail(reason="Issue #1118", strict=True)
@pytest.mark.filterwarnings("ignore") # Test incorrectly warns
class TestErrorInCloseImportedBuffer(ChildErrorHarness):
"""Test that errors when closing an imported buffer are raised."""

def PARENT_ACTION(self, queue):
self.buffer = self.mr.allocate(NBYTES)
queue.put(self.buffer)

def CHILD_ACTION(self, queue):
buffer = queue.get(timeout=CHILD_TIMEOUT_SEC)
try:
driver.cuMemFree(buffer.handle)
except Exception: # noqa: S110
pass
else:
buffer.close()

def ASSERT(self, exc_type, exc_msg):
assert exc_type is CUDAError
assert re.match(r"CUDA_ERROR_INVALID_VALUE", exc_msg)