Repository navigation
Serialize bytearray exports with resizing #9087
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.
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
Changes from 1 commit
d0819a2
fcd4f4c
9fdae0a
a0fec1f
f163e13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,11 +173,6 @@ impl PyByteArray { | |
| fn inner(&self) -> PyDetachingRwLockReadGuard<'_, PyBytesInner> { | ||
| self.inner.read() | ||
| } | ||
| #[inline] | ||
| fn inner_mut(&self) -> PyDetachingRwLockWriteGuard<'_, PyBytesInner> { | ||
| self.inner.write() | ||
| } | ||
|
|
||
| fn __len__(&self) -> usize { | ||
| self.borrow_buf().len() | ||
| } | ||
|
|
@@ -781,9 +776,18 @@ impl Initializer for PyByteArray { | |
| type Args = ByteInnerNewOptions; | ||
|
|
||
| fn init(zelf: &Py<Self>, options: Self::Args, vm: &VirtualMachine) -> PyResult<()> { | ||
| // First unpack bytearray and *then* get a lock to set it. | ||
| let mut inner = options.get_inner(bytearray_from_object, vm)?; | ||
| core::mem::swap(&mut *zelf.inner_mut(), &mut inner); | ||
| // Reject exports and clear existing contents before converting source. | ||
| zelf.try_resizable(vm)?.elements.clear(); | ||
| // Conversion can call Python, so do not hold the lock across it and | ||
| // check again for exports acquired by a callback or another thread. | ||
| let (mut inner, zero_count) = | ||
| options.get_inner_with_zero_count(bytearray_from_object, vm)?; | ||
| // A zero integer count performs no further writes, including when its | ||
| // __index__ callback changed this bytearray or acquired an export. | ||
| if zero_count { | ||
| return Ok(()); | ||
|
Comment on lines
+935
to
+936
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a large existing bytearray is reinitialized with an integer (or int-like value) of zero, this early return follows Useful? React with 👍 / 👎. |
||
| } | ||
| core::mem::swap(&mut *zelf.try_resizable(vm)?, &mut inner); | ||
|
Comment on lines
+935
to
+938
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| Ok(()) | ||
| } | ||
| } | ||
|
|
@@ -841,9 +845,11 @@ impl AsBuffer for PyByteArray { | |
| } | ||
|
|
||
| fn as_buffer(zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<PyBuffer> { | ||
| // Keep the length stable until PyBuffer::new retains the export. | ||
| let inner = zelf.inner(); | ||
| Ok(PyBuffer::new( | ||
| zelf.to_owned().into(), | ||
| BufferDescriptor::simple(zelf.__len__(), false), | ||
| BufferDescriptor::simple(inner.elements.len(), false), | ||
| &BUFFER_METHODS, | ||
| )) | ||
| } | ||
|
|
@@ -855,7 +861,12 @@ impl BufferResizeGuard for PyByteArray { | |
| fn try_resizable_opt(&self) -> Option<Self::Resizable<'_>> { | ||
| // An export is a borrow someone else still holds, so it is answered | ||
| // before the lock rather than by waiting on it. | ||
| (self.exports.load(Ordering::SeqCst) == 0).then(|| self.inner.write()) | ||
| if self.exports.load(Ordering::SeqCst) != 0 { | ||
| return None; | ||
| } | ||
| let inner = self.inner.write(); | ||
| // An export may have been acquired while we waited for the lock. | ||
| (self.exports.load(Ordering::SeqCst) == 0).then_some(inner) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -907,7 +918,7 @@ impl AsSequence for PyByteArray { | |
| if let Some(value) = value { | ||
| zelf._setitem_by_index(i, &value, vm) | ||
| } else { | ||
| zelf.borrow_buf_mut().delitem_by_index(vm, i) | ||
| zelf.try_resizable(vm)?.elements.delitem_by_index(vm, i) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a C extension calls AGENTS.md reference: AGENTS.md:L7-L7 Useful? React with 👍 / 👎. |
||
| } | ||
| }), | ||
| contains: atomic_func!(|seq, other, vm| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| """Resizing paths must respect outstanding bytearray exports.""" | ||
|
|
||
| import io | ||
|
|
||
| from testutils import assert_raises | ||
|
|
||
| for args in ((), (b"abc",), (b"x",), (b"",), (object(),)): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Remove this new regression snippet and the two sibling AGENTS.md reference: AGENTS.md:L273-L279 Useful? React with 👍 / 👎. |
||
| value = bytearray(b"abc") | ||
| with memoryview(value) as view: | ||
| with assert_raises(BufferError): | ||
| value.__init__(*args) | ||
| assert value == b"abc" | ||
| assert view.tobytes() == b"abc" | ||
| value.__init__(b"released") | ||
| assert value == b"released" | ||
|
|
||
| value = bytearray(b"abc") | ||
| with assert_raises(TypeError): | ||
| value.__init__(object()) | ||
| assert value == b"" | ||
| value = bytearray(b"abc") | ||
| value.__init__(value) | ||
| assert value == b"" | ||
|
|
||
|
|
||
| class ExportDuringInit: | ||
| def __init__(self, count, payload=b""): | ||
| self.count = count | ||
| self.payload = payload | ||
| self.calls = 0 | ||
|
|
||
| def __index__(self): | ||
| self.calls += 1 | ||
| value.extend(self.payload) | ||
| self.view = memoryview(value) | ||
| return self.count | ||
|
|
||
|
|
||
| value = bytearray(b"abc") | ||
| source = ExportDuringInit(2) | ||
| with assert_raises(BufferError): | ||
| value.__init__(source) | ||
| assert value == b"" | ||
| assert source.view.tobytes() == b"" | ||
| assert source.calls == 1 | ||
| source.view.release() | ||
| value.extend(b"released") | ||
| assert value == b"released" | ||
|
|
||
| value = bytearray(b"abc") | ||
| source = ExportDuringInit(0) | ||
| value.__init__(source) | ||
| assert value == b"" | ||
| assert source.view.tobytes() == b"" | ||
| source.view.release() | ||
|
|
||
| value = bytearray(b"abc") | ||
| source = ExportDuringInit(0, b"xy") | ||
| value.__init__(source) | ||
| assert value == b"xy" | ||
| assert source.view.tobytes() == b"xy" | ||
| assert source.calls == 1 | ||
| source.view.release() | ||
|
|
||
|
|
||
| class FailingIndex: | ||
| def __index__(self): | ||
| assert value == b"" | ||
| raise ValueError("conversion failed") | ||
|
|
||
|
|
||
| value = bytearray(b"abc") | ||
| with assert_raises(ValueError): | ||
| value.__init__(FailingIndex()) | ||
| assert value == b"" | ||
|
|
||
| for key in (0, slice(0, 1)): | ||
| value = bytearray(b"abc") | ||
| with memoryview(value) as view: | ||
| with assert_raises(BufferError): | ||
| del value[key] | ||
| assert view.tobytes() == b"abc" | ||
| del value[key] | ||
| assert value == b"bc" | ||
|
|
||
|
|
||
| class Reader(io.RawIOBase): | ||
| def __init__(self, count, retain, payload=b"abcd"): | ||
| self.count = count | ||
| self.retain = retain | ||
| self.payload = payload | ||
|
|
||
| def readinto(self, buffer): | ||
| buffer[:] = self.payload | ||
| self.buffer = buffer | ||
| if self.retain: | ||
| self.view = memoryview(buffer) | ||
| return self.count | ||
|
|
||
|
|
||
| for count in (0, 2, 4): | ||
| reader = Reader(count, False) | ||
| assert reader.read(4) == b"abcd"[:count] | ||
| assert reader.buffer == b"" | ||
|
|
||
| reader = Reader(count, True) | ||
| with assert_raises(BufferError): | ||
| reader.read(4) | ||
| assert reader.view.tobytes() == b"abcd" | ||
| reader.view.release() | ||
| reader.buffer.clear() | ||
|
|
||
| for count in (-1, 5): | ||
| reader = Reader(count, True) | ||
| with assert_raises(ValueError): | ||
| reader.read(4) | ||
| assert reader.view.tobytes() == b"abcd" | ||
| reader.view.release() | ||
|
|
||
| reader = Reader(None, True) | ||
| assert reader.read(4) is None | ||
| assert reader.view.tobytes() == b"abcd" | ||
| reader.view.release() | ||
|
|
||
| # The callback can shrink the temporary buffer, so slicing by the original | ||
| # requested size without checking the current buffer is not safe. | ||
| reader = Reader(2, False, b"xy") | ||
| assert reader.read(4) == b"xy" | ||
| assert reader.buffer == b"" | ||
|
|
||
| reader = Reader(4, False, b"xy") | ||
| result = reader.read(4) | ||
| assert len(result) == 4 | ||
| assert result[:2] == b"xy" | ||
| # The contents of the newly grown region are unspecified by CPython. | ||
| assert reader.buffer == b"" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| """Acquiring a bytearray export must be atomic with respect to resizing.""" | ||
|
|
||
| import threading | ||
|
|
||
|
|
||
| def exercise_exports(): | ||
| payload = b"x" * 65536 | ||
| value = bytearray(payload) | ||
| barrier = threading.Barrier(3, timeout=5) | ||
| errors = [] | ||
|
|
||
| def worker(operation): | ||
| try: | ||
| for _ in range(256): | ||
| barrier.wait() | ||
| operation() | ||
| barrier.wait() | ||
| except BaseException as exc: | ||
| errors.append(exc) | ||
| barrier.abort() | ||
|
|
||
| def resize(): | ||
| try: | ||
| value.clear() | ||
| value.extend(payload) | ||
| except BufferError: | ||
| pass | ||
|
|
||
| def decode(): | ||
| result = value.decode("ascii") | ||
| assert result in ("", "x" * len(payload)) | ||
|
|
||
| def export(): | ||
| with memoryview(value) as view: | ||
| size = len(view) | ||
| assert size in (0, len(payload)) | ||
| # The acquired export keeps its original length and contents even | ||
| # if the resizing thread runs between these two reads. | ||
| assert view.tobytes() == payload[:size] | ||
| assert len(view) == size | ||
|
|
||
| threads = [ | ||
| threading.Thread(target=worker, args=(operation,), daemon=True) | ||
| for operation in (resize, decode, export) | ||
| ] | ||
| for thread in threads: | ||
| thread.start() | ||
| for thread in threads: | ||
| thread.join(timeout=10) | ||
| assert not any(thread.is_alive() for thread in threads), "worker did not finish" | ||
| if errors: | ||
| raise errors[0] | ||
|
|
||
|
|
||
| exercise_exports() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an empty bytearray already has a live
memoryview,__init__()and__init__(0)perform no resize; CPython permits both and still invokes conversion callbacks for an int-like zero. The unconditional resize guard here instead raisesBufferErrorbefore options conversion, regressing the previously successful no-op and suppressing those callbacks. Skip the initial guard/clear when the current length is zero and only reject if the eventual result actually needs a resize.AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.