Repository navigation
Conversation
…xtend()
`google_crc32c.value()`, `extend()` and `Checksum.update()` parsed their
chunk with `PyArg_ParseTuple("y#")`, which only accepts objects without a
`releasebuffer` slot, so `memoryview`, `bytearray` and `array.array` were
rejected with "argument must be read-only bytes-like object". Callers that
hold a view into a larger buffer had to copy it into `bytes` first just to
checksum it.
Parse the chunk as a `Py_buffer` ("y*") instead, which accepts any
C-contiguous object supporting the buffer protocol, and release it once the
checksum is computed. The GIL is still released for chunks of at least
1 MiB, now for any read-only buffer (`bytes`, or a read-only view) rather
than only `bytes`; mutable buffers such as `bytearray` keep the GIL as
before. Non-contiguous views raise `BufferError`, as with `hashlib`.
Add a non-zero-offset memoryview slice and a writable bytearray-backed view to the bytes-like parametrization so the extension is shown to hash the exported slice rather than the exporter's whole buffer, test the Checksum constructor with every bytes-like type, check that extend() rejects non-contiguous views like value() does, and align the _checksum.py / python.py docstrings with cext.py.
Checksum.update() converted its input with array.array("B", data),
which iterates items rather than bytes. Multi-byte buffers such as
array("H") or memoryview.cast("I") therefore hashed differently from the
C extension (or raised OverflowError), and mmap, 'c'-format and N-D
buffers raised, although the C extension hashes their raw bytes.
Hash memoryview(data).cast("B") for buffer objects, raise BufferError
for views that are not C-contiguous as the C extension does, and keep
array.array("B", data) only for iterables of ints.
The extension released the GIL for any read-only chunk of at least 1 MiB, but a read-only buffer is not necessarily immutable: the owner of memoryview(bytearray).toreadonly(), or of a numpy array with writeable=False, can still write to it from another thread, so the checksum could cover a mix of old and new contents. Release the GIL only when the memory belongs to a bytes object, passed directly or through a memoryview (including slices) of it, so slices of large bytes payloads are still hashed without the GIL. All other buffers are hashed with the GIL held, as bytearray already was.
The tests proved only the negative half of the GIL rule: value() keeps the
GIL for a bytearray and for read-only views of one. The property the storage
SDK's off-loop checksum depends on, that hashing a bytes object or a
memoryview into one of 1 MiB or more runs with the GIL released, had no test.
The same probe thread now checks that it runs while value() is in progress for
bytes, a memoryview of bytes and a slice of that view.
The numpy-only line of the Fortran-order helper is excluded from coverage; the
unit session does not install numpy, so it skips before that line and the
cover session would otherwise stop at 99%.
Test plan: with the extension built from this worktree
(google_crc32c.implementation == "c") and with CRC32C_PURE_PYTHON=1:
python -m pytest packages/google-crc32c/tests --cov=google_crc32c --cov=tests \
--cov-config=.coveragerc --cov-report=term-missing
180 pass, 4 skipped (numpy), coverage 100%; the three new cases pass on three
consecutive runs. ruff check/format (0.14.14) and flake8 are clean.
Authored with an AI assistant.
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds support for bytes-like objects (such as bytearray, memoryview, array.array, and mmap) in both the C extension and pure-Python implementations of the CRC32C checksum. It updates argument parsing to accept any C-contiguous object supporting the buffer protocol and refactors GIL release logic to safely verify buffer immutability. Additionally, comprehensive tests are introduced to validate these changes. Feedback on the C extension suggests using a recursive loop to traverse nested memoryview wrappers to ensure the GIL is correctly released for multi-layered views.
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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
google_crc32c.value(),extend()andChecksum.update()accept any contiguous bytes-like object (memoryview,bytearray,array,bytes) instead ofbytesonly, and the C extension releases the GIL for inputs of 1 MiB or more whose memory belongs to abytesobject (abytesor amemoryviewof one). No API change.Motivation: the storage client wants to verify chunks it receives as
memoryviewslices of the gRPC message without copying them intobytesfirst; todayvalue(memoryview(...))raisesTypeError: argument 1 must be read-only bytes-like object.Changes
src/google_crc32c/_crc32c.c: parse the data argument withy*into aPy_buffer(released after hashing) instead ofy#, which rejects every exporter with a release function. The 1 MiB GIL release applies when the exported memory is owned by abytesobject; abytearrayor other writable owner keeps the GIL, since another thread could write to it mid-hash.src/google_crc32c/python.py: the pure-Python implementation hashes the raw bytes of the same inputs (a multi-bytearrayor cast view is hashed byte for byte, as the C extension does, instead of item by item).cext.py,_checksum.pyandpython.pysay bytes-like.tests/test___init__.py:value/extendonbytes,bytearray,memoryview(sliced, writable, read-only, cast, 2-D) andarrayinputs,BufferErroron non-contiguous views, and GIL probes: held forbytearrayand read-only views of it, released forbytes,memoryview(bytes)and a slice of it.Tests
180 passed, 4 skipped (numpy not installed), 100% coverage, with the compiled extension and with
CRC32C_PURE_PYTHON=1.Notes
array/cast inputs (previously coerced item by item). It now matches the C extension.bytes-owned memory only) is a choice:hashlibandzlib.crc32release the GIL for any buffer. Holding it for writable owners avoids hashing memory another thread may be writing to; happy to drop that restriction if you prefer parity withhashlib.