Make streaming multipart decode O(n)

[thiromi]

Fixes #127

Streaming _decode_multipart_message now resumes find() across chunks instead of rescanning and copying the whole buffer on every chunk.

running with uv run pytest tests/test_streaming.py -s -v you can see the comparison over buffered vs streaming

tests/test_streaming.py::test_streaming_single_part_decode_is_linear[1MB-payload-32KB-chunk] streamed 0.002s vs buffered 0.006s (0.3x)
PASSED
tests/test_streaming.py::test_streaming_single_part_decode_is_linear[5MB-payload-32KB-chunk] streamed 0.004s vs buffered 0.005s (0.7x)
PASSED
tests/test_streaming.py::test_streaming_single_part_decode_is_linear[10MB-payload-32KB-chunk] streamed 0.006s vs buffered 0.010s (0.6x)
PASSED
tests/test_streaming.py::test_streaming_single_part_decode_is_linear[15MB-payload-32KB-chunk] streamed 0.008s vs buffered 0.014s (0.5x)
PASSED
tests/test_streaming.py::test_streaming_single_part_decode_is_linear[20MB-payload-32KB-chunk] streamed 0.012s vs buffered 0.026s (0.5x)
PASSED
tests/test_streaming.py::test_streaming_single_part_decode_is_linear[40MB-payload-32KB-chunk] streamed 0.023s vs buffered 0.035s (0.7x)
PASSED

before the change

FAILED tests/test_streaming_perf.py::test_streaming_single_part_decode_is_linear[5MB-payload-32KB-chunk] - AssertionError: streaming decode was 3.4x slower than buffered (0.041s vs 0.012s); expected <2x for O(n) scaling
FAILED tests/test_streaming_perf.py::test_streaming_single_part_decode_is_linear[10MB-payload-32KB-chunk] - AssertionError: streaming decode was 6.0x slower than buffered (0.117s vs 0.020s); expected <2x for O(n) scaling
FAILED tests/test_streaming_perf.py::test_streaming_single_part_decode_is_linear[15MB-payload-32KB-chunk] - AssertionError: streaming decode was 10.3x slower than buffered (0.265s vs 0.026s); expected <2x for O(n) scaling
FAILED tests/test_streaming_perf.py::test_streaming_single_part_decode_is_linear[20MB-payload-32KB-chunk] - AssertionError: streaming decode was 20.6x slower than buffered (0.620s vs 0.030s); expected <2x for O(n) scaling

[pieper]

Generally this looks good to go - thanks for the fix!

Co-pilot made a few suggestions. I won't paste them in here but you could ask it yourself and see if you agree. Mostly style and a few edge performance issues.

[thiromi]

@pieper sure! Do you mind if I ask where those suggestions are?

[pieper]

Here you go. Like I said, I don't think it's too critical but probably worth having a look.

Copilot Chat Conversation Export: Reviewing PR for red flags and

User: @pieper
Thread URL: https://github.com/copilot/c/903841fa-acaf-49c4-ad88-f97bd7341122

@pieper

Any red flags in this PR?

Copilot

Summary — quick answer

• No correctness red flags jump out: the change converts a rescanning O(n^2) loop into a single-pass search by resuming find() across chunk boundaries and includes a new test that verifies streaming decode stays O(n) for a large single part.
• I did find a few maintainability/perf/test robustness improvements and two small opportunities to harden the implementation and tests (see actionable items below).

What I did

• Loaded the PR reviewer ability and fetched the PR metadata and the two changed files (src/dicomweb_client/web.py and tests/test_streaming.py).
• Read the patch and the new test that compares streaming vs buffered decoding times.

PR summary

• Title: Make streaming multipart decode O(n)
• Author: thiromi (first-time contributor)
• Files changed: 2 (web.py modified, tests/test_streaming.py added)
• Net additions: ~91 lines (test + small fix)
• Intent: Avoid repeated rescanning/copying of the buffer while decoding streaming multipart messages; instead resume the find() from where the last search left off so streaming is linear-time.
• Linked issue: Fixes _decode_multipart_message is O(n²) when streaming a single large part #127
• Risk: low-to-medium — change is localized, but touches a streaming decoder used by I/O paths. New tests exercise a performance property but CI/test coverage for edge cases is limited.

Core change (what and why)

• In src/dicomweb_client/web.py, the code previously did data.find(delimiter, prev_part_index) and re-sliced the entire buffer on every chunk, which caused O(n^2) behavior when the buffer grew.
• The patch introduces search_from which keeps the start index for find() across chunks, updates search_from to the new prev_part_index after consuming parts, and after trimming the buffer computes a restart index near the buffer tail so split-delimiter matches aren't missed.
• This avoids re-scanning the already-searched prefix and greatly reduces work done per chunk.

Relevant snippets (context from the PR)

         delimiter = b''.join((b'\r\n', marker))
         data = bytearray()
         j = 0
+        search_from = 0
         with response:
             logger.debug('decode message content')
             if stream:
@@
-                prev_part_index = 0
-                while True:
-                    delimiter_index = data.find(delimiter, prev_part_index)
+                prev_part_index = 0
+                while True:
+                    delimiter_index = data.find(delimiter, search_from)
                     if delimiter_index < 0:
                         break
@@
                     prev_part_index = delimiter_index + len(delimiter)
+                    search_from = prev_part_index
                     j += 1
@@
-                data = data[prev_part_index:]
+                if prev_part_index:
+                    data = data[prev_part_index:]
+                search_from = max(0, len(data) - len(delimiter) + 1)

Other change: new test file

# tests/test_streaming.py added: a parametrized test that builds a single large multipart body,
# then compares time for buffered (stream=False) vs streamed (stream=True) retrieval,
# asserting streamed is < 2x slower than buffered up to 40MB payloads.

Merge readiness and risk assessment

• Code quality: good — the patch is small and targeted; variable naming is clear enough.
• Tests: a helpful performance test was added for single large-part decoding.
• Blockers: none obvious in code; PR is marked mergeable but mergeable_state = "blocked" (likely CI or approvals needed).
• Risk rating: Low-to-medium — behavior change in a streaming parser; could affect edge cases where boundaries split across chunks or where multiple parts are present (the new test covers only a single-part case).
• Recommended non-blocking chores (not inline comments): ensure CI runs and passes; add a short PR description of the strategy (briefly explaining why search_from is safe/what scenario it fixes).

Actionable review comments

• src/dicomweb_client/web.py: Consider replacing the copy operation data = data[prev_part_index:] with an in-place trim (e.g. del data[:prev_part_index]) to avoid allocating a new bytearray each time the buffer is trimmed — this keeps the algorithm O(n) and can reduce peak allocations.
• tests/test_streaming.py: The timing-based assertion (ratio < 2x) is useful but can be flaky on CI; make the test less timing-sensitive (for example, compare operation counts or use a deterministic micro-benchmark, or relax the ratio, or assert that streamed time scales linearly by checking ratio remains approximately constant across parametrized sizes).
• src/dicomweb_client/web.py: Add a short comment above the search_from logic describing why find() resumes from search_from and why search_from is adjusted after trimming (note: this helps future maintainers understand the boundary-splitting rationale).

Possible improvements (suggestions tied to code/tests)

• src/dicomweb_client/web.py: Add unit tests that specifically exercise:
  • a delimiter split across two chunks (the classic boundary-split case),
  • multiple parts in a single stream (not only the single large part).
    These should assert correctness (content matches) rather than timings to be robust in CI.
• src/dicomweb_client/web.py: Consider renaming search_from → search_start (minor readability).
• tests/test_streaming.py: Instead of directly timing real wall-clock durations, you could (a) simulate chunk delivery in a tight loop and count find() invocations (if instrumentable), or (b) add an environment guard so timing-based assertions are skipped/loosened on heavily-loaded CI runners.

Notes on edge cases and potential risks

• The algorithm still buffers an entire part if there's no delimiter — that's expected: you cannot emit the part until you see the next delimiter (or end-of-stream); the change only reduces repeated rescans.
• The current search_from logic seems correct for avoiding missed matches, thanks to the final line that sets search_from = max(0, len(data) - len(delimiter) + 1) after trimming. I did not find an obvious correctness bug there.
• Consider very long delimiters (unlikely for typical multipart boundaries) — the logic uses len(delimiter) appropriately.

Small stylistic nit

• The new local var search_from is fine; a one-line comment would be helpful: "# resume find() from previous search position to avoid rescanning the whole buffer".

Want me to…

1. Run a focused test matrix (unit tests + the new timing test) on CI and report if any timing flakiness occurs.
2. Add the two deterministic correctness tests (delimiter-split-across-chunk and multiple-parts) and update the PR — I can draft the test code.
3. Send a tiny follow-up PR suggestion to replace data = data[prev_part_index:] with del data[:prev_part_index] (or an alternate in-place approach) and include a short comment explaining the logic.

If you want, I can implement suggestion 2 (draft the extra tests) and/or suggestion 3 and push a small follow-up patch here in the PR. Which would you like me to do next?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[thiromi]

@pieper applied most of the changes. The timing thing I don't think it applies now, as the tests are rather quick (actually streaming is now quicker than buffering by 2x 😛), and as the test compares the time one takes against the other, even on high loads they'll tend to be the same. I removed the obvious cases (payloads <= 15MB) and only left the hefty ones.

Thanks for the review 🙏

[thiromi]

Hi @pieper

kind reminder about the status of this PR's code review 🙏

[pieper]

github had some issues but it seems to be back now.

@CPBridge could you take a quick look at this?

[CPBridge]

Looks good to me, thanks @thiromi !

[CPBridge]

@pieper if you are onboard we can merge and put out a patch release

[pieper]

Thanks @CPBridge 👍