)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ee70e23aedc32f2160c42af7d92addd371cb615d","unresolved":true,"context_lines":[{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Treat an empty chunk read before the terminal zero-byte chunk completes"},{"line_number":14,"context_line":"the payload as incomplete input instead. Add parser-level and object PUT"},{"line_number":15,"context_line":"regressions for a client disconnecting mid-chunk."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: I0fb8aa85232b8cea37fa01863f777233dd50af8e"},{"line_number":18,"context_line":"Signed-off-by: Clay Gerrard \u003cclay.gerrard@gmail.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"891be72b_5dde33e9","line":15,"range":{"start_line":15,"start_character":0,"end_line":15,"end_character":11},"updated":"2026-05-11 17:08:15.000000000","message":"s/regressions/tests/","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"4bc4a344a21ad6a5e0e62e3b6972898d792b0acb","unresolved":false,"context_lines":[{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Treat an empty chunk read before the terminal zero-byte chunk completes"},{"line_number":14,"context_line":"the payload as incomplete input instead. Add parser-level and object PUT"},{"line_number":15,"context_line":"regressions for a client disconnecting mid-chunk."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: I0fb8aa85232b8cea37fa01863f777233dd50af8e"},{"line_number":18,"context_line":"Signed-off-by: Clay Gerrard \u003cclay.gerrard@gmail.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"710da2a9_a0e5efa9","line":15,"range":{"start_line":15,"start_character":0,"end_line":15,"end_character":11},"in_reply_to":"891be72b_5dde33e9","updated":"2026-05-14 08:40:34.000000000","message":"Done","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ee70e23aedc32f2160c42af7d92addd371cb615d","unresolved":true,"context_lines":[{"line_number":15,"context_line":"regressions for a client disconnecting mid-chunk."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: I0fb8aa85232b8cea37fa01863f777233dd50af8e"},{"line_number":18,"context_line":"Signed-off-by: Clay Gerrard \u003cclay.gerrard@gmail.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"960b3497_5dd7dd29","line":18,"updated":"2026-05-11 17:08:15.000000000","message":"I think a launchpad bug would be appropriate for this","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ee70e23aedc32f2160c42af7d92addd371cb615d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"de9d59b3_7961ce75","updated":"2026-05-11 17:08:15.000000000","message":"Kudos for tracking this down 👏\n\nI\u0027d be happy for this to merge as-is in order to fix the bug ASAP (modulo the commit message changes). That said, I wonder if the bug should actually be addressed in ChunkReader, which should probably have eof checking similar to the HashingInput (which is used for non-streaming), since ChunkReader is a thing in itself that should behave correctly. But, even if appropriate and correct, that isn\u0027t essential IMO vs getting a fix shipped. I may work up a follow on patch, but repeat: I\u0027m happy for this to merge.\n\nAFAICT the bug defeats the client timeout that would be applied further down the read chain (in proxy obj controller) because IIUC that would rely on the green thread yielding and, again IIUC (caveat: I\u0027m still digging), the GreenSocket won\u0027t trampoline and will return b\u0027\u0027 if the socket is closed. So the reading thread gets into a tight loop.","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":30073,"name":"Brendan Shephard","email":"bshephar@bne-home.net","username":"bshephar"},"change_message_id":"c12764b27ced09a809cb88352eab3cb05f0b5b24","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"487fb1b4_c0a20833","updated":"2026-05-08 22:34:00.000000000","message":"Nice catch man!","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"9cb5d6d6f21df89899547e2a7ee2971a0a894609","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"cecf199b_6ac7e736","updated":"2026-05-11 11:12:20.000000000","message":"Tested the fix on a SAIO as well. LGTM.","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"d964b645419745c4d7ebadc015c5d32f88a5a8e0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"7061f914_9dd5c93e","updated":"2026-05-11 03:16:05.000000000","message":"Yup, stops the infinite loop for aws-chunked requests that end in the middle of an expected chunk, and this doesn\u0027t mess with the expected empty chunk that is expected to be sent, because there is already a check for it via the chunk reader expected chunk size. So a valid and expected empty chunk will set the `self._completed_payload`.\n\nNice one Clay.","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"33303b0838ed48d83dc5a2879be36d5eda96fe63","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e57edea9_4edb7ccf","updated":"2026-05-08 23:28:35.000000000","message":"recheck\n\nI\u0027m pretty sure the tempauth post failure is unrelated; I didn\u0027t see any logs/output when I tried to click through on it","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"dfca5b64993475e1f84e9677e4d6c2eb6cd029d9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"8f5c7700_ee10fd76","in_reply_to":"de9d59b3_7961ce75","updated":"2026-05-11 19:45:32.000000000","message":"\u003e the bug should actually be addressed in ChunkReader\n\nI tried this a couple of times different ways, the main difference between the other sub-classes of `InputProxy` is that a `ChunkReader` only knows how long it\u0027s *chunk* is (as opposed to the whole stream).\n\nIt can (should?) do the same \"update and also eof check\" as the others:\n\n```\n        if self.bytes_received \u003c self._expected_length:\n            error \u003d eof\n```\n\n... but currently those classes all raise `S3InputChunkSignatureMismatch` and in this case we really want `S3InputIncomplete` (which is currently the domain of `StreamingInput`).\n\nSo I landed here as the simplest/cleanest/targeted fix.  It\u0027s close to the `bufs.append(buf)` layer that has the memory leak but also because `if not buf: break` is sort a \"normal looking pattern\" and the `if bytes_receieved \u003c expected_lenght: if eof: raise signature-mistmatch` seemed like a really akward/less-familiar way to exit a read loop.\n\n\u003e the reading thread gets into a tight loop\n\nsure does, nasty stuff.  I think we could teach InputProxy to return one-and-only-one b\u0027\u0027 after which it explodes - a traceback would have been significantly better than an infinite loop.  But that would be a more intrustive interface chance, and we\u0027d still want to raise `S3InputIncomplete` so I think this is minimum necessary and sufficient fix.","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"dfca5b64993475e1f84e9677e4d6c2eb6cd029d9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"2e812e07_8334ba04","updated":"2026-05-11 19:45:32.000000000","message":"I agree probably useful to have a lp bug in-case someone else runs into this they should be able to find which swift versions have a fix.","commit_id":"5ad23e2f6b91f75a8fe0008c8dfc9fb3d4bb6ca3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"4bc4a344a21ad6a5e0e62e3b6972898d792b0acb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3158e85a_59b7545e","updated":"2026-05-14 08:40:34.000000000","message":"re-adding my vote to avoid any doubt","commit_id":"5ad23e2f6b91f75a8fe0008c8dfc9fb3d4bb6ca3"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"1fcbbc45300061403f1088c57d3d67d37a7c3720","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e1c339b0_b53001ab","updated":"2026-05-27 13:51:04.000000000","message":"recheck\n\nhttps://review.opendev.org/c/openstack/swift/+/990252 merged that fixes the 3.10 failure.","commit_id":"58918e18e7863fc38c858075a8097ef1dcea3b7d"}],"swift/common/middleware/s3api/s3request.py":[{"author":{"_account_id":30073,"name":"Brendan Shephard","email":"bshephar@bne-home.net","username":"bshephar"},"change_message_id":"c12764b27ced09a809cb88352eab3cb05f0b5b24","unresolved":true,"context_lines":[{"line_number":445,"context_line":"            self._to_read -\u003d len(buf)"},{"line_number":446,"context_line":"            if readline and buf[-1:] \u003d\u003d b\u0027\\n\u0027:"},{"line_number":447,"context_line":"                break"},{"line_number":448,"context_line":"            if not buf and not self._completed_payload:"},{"line_number":449,"context_line":"                raise S3InputIncomplete"},{"line_number":450,"context_line":"        return b\u0027\u0027.join(bufs)"},{"line_number":451,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"95cdeba7_08f1adf0","line":448,"updated":"2026-05-08 22:34:00.000000000","message":"Wouldn\u0027t you want to do this earlier? Does all of the checking on L436-L445 provide any value if we\u0027re just going to raise here? Couldn\u0027t we move this new addition to L435 before buf.append() and raise a bit earlier after we\u0027ve finished collecting `buf`?\n\nMaybe like:\n```\n  if readline:\n    buf \u003d self._chunk_reader.readline(size - bytes_read)\n  else:\n    buf \u003d self._chunk_reader.read(size - bytes_read)\n  if not buf and self._chunk_reader.chunk_size !\u003d 0:\n      raise S3InputIncomplete\n  bufs.append(buf)\n  if self._chunk_reader.to_read \u003d\u003d 0:\n```","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":30073,"name":"Brendan Shephard","email":"bshephar@bne-home.net","username":"bshephar"},"change_message_id":"4f505152ab954b602771aef92fdb1d6260cf356d","unresolved":false,"context_lines":[{"line_number":445,"context_line":"            self._to_read -\u003d len(buf)"},{"line_number":446,"context_line":"            if readline and buf[-1:] \u003d\u003d b\u0027\\n\u0027:"},{"line_number":447,"context_line":"                break"},{"line_number":448,"context_line":"            if not buf and not self._completed_payload:"},{"line_number":449,"context_line":"                raise S3InputIncomplete"},{"line_number":450,"context_line":"        return b\u0027\u0027.join(bufs)"},{"line_number":451,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1d2f00be_3b1e096a","line":448,"in_reply_to":"20a9aca2_7bbe8cfe","updated":"2026-05-08 23:56:32.000000000","message":"Yeah, apologies I unintentionally changed the if statement. I only included it for illustrative purposes to demonstrate the positioning I was proposing. The main point of my comment was to inquire about the usefulness of the preceding code. If we had no use for it, then it would be unnecessary CPU cycles. But I think you have answered this when you said the value is in keeping the control logic and retaining the primary exit mechanism.\n\nThanks for the clarification.","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"33303b0838ed48d83dc5a2879be36d5eda96fe63","unresolved":true,"context_lines":[{"line_number":445,"context_line":"            self._to_read -\u003d len(buf)"},{"line_number":446,"context_line":"            if readline and buf[-1:] \u003d\u003d b\u0027\\n\u0027:"},{"line_number":447,"context_line":"                break"},{"line_number":448,"context_line":"            if not buf and not self._completed_payload:"},{"line_number":449,"context_line":"                raise S3InputIncomplete"},{"line_number":450,"context_line":"        return b\u0027\u0027.join(bufs)"},{"line_number":451,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"20a9aca2_7bbe8cfe","line":448,"in_reply_to":"95cdeba7_08f1adf0","updated":"2026-05-08 23:28:35.000000000","message":"personally I find `and not self._complete_playload` rolls of the tongue a little better than `self._chunk_reader.chunk_size !\u003d 0` (\"not !\u003d 0\"?  \"is \u003d\u003d 0\"?  \"\u003c\u003d 0\"? or... not?)\n\nI also like the symmetry of the the condition that terminates the loop early (with an error) being at the END of a `while` loop that is predicated on the *same* condition `while not self._completed_payload` signaling clearly that `if [not] buf *and* completed_playload\" we\u0027re going to exit anyway.\n\n\u003e Wouldn\u0027t you want to do this earlier?\n\nno I like it where I put it; other implementations could also make the new tests pass tho - and that\u0027s probably the main thing that matters.  CRITICAL.  BUG.  FIX.  The rest is just non-material personal preference; beauty is in the eye and all that:\n\nhttps://github.com/NVIDIA/swift/blob/master/REVIEW_GUIDELINES.rst#maintainable-code-is-obvious\n\n\u003e Does all of the checking on L436-L445 provide any value\n\nI think the value is it keeps the existing logic for `_completed_playload` un-disturbed, which I think is a good thing *if* we\u0027re trying to keep `self._completed_playload` as the loop\u0027s *primary* exit/control interface.\n\n\u003e Couldn\u0027t we move ... raise a bit earlier\n\ncorrect, other diffs could also fix the included tests.  this one is mine.  ;)\n\nI think your logic is sound, but i\u0027m not sure I understand your reasoning.  It feels less like \"I think this is more readable/maintainable/obvious because XYZ\" and more like \"just confirming there\u0027s nothing special about the placement; the main thing is we need to detect when \"empty buff is bad\" (i.e. anytime the playload is not complete) and raise an error, correct?\" - in which case: \"correct\"\n\nThis was the most obvious way I could find to spell: \"it\u0027s a bug if this loop does not exit when when payload complete OR if buff is empty and NOT payload complete\"\n\nI could have also gone with `if not (buf or complete)` - i wrote it the way I did on purpose; not because it makes the computer happier (the computer doesn\u0027t care about readability/maintainability) but because it makes ME happiest (*I* care).\n\nAs far as \"but exit sooner is more *efficient*\" - this is already a hot loop and this is a special error condition; I don\u0027t care about `len(0)` running right before I raise an error.  I care about reading/understanding/maintaining the correct behavior of this parser for another 10 years w/o forgetting to raise an error on empty buff and payload not complete.  Using `_complete_payload` *leverages* an abstraction instead of breaking it.   Doing the check before continuing the loop re-enforces the special error condition and the guarantee of safe termination by *emphasizing* the termination rather than burying it w/ other logic.  That\u0027s my thinking at least...","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"50c370fc345a43ceafbe506414cb586af4fc1623","unresolved":true,"context_lines":[{"line_number":431,"context_line":"            if readline:"},{"line_number":432,"context_line":"                buf \u003d self._chunk_reader.readline(size - bytes_read)"},{"line_number":433,"context_line":"            else:"},{"line_number":434,"context_line":"                buf \u003d self._chunk_reader.read(size - bytes_read)"},{"line_number":435,"context_line":"            bufs.append(buf)"},{"line_number":436,"context_line":"            if self._chunk_reader.to_read \u003d\u003d 0:"},{"line_number":437,"context_line":"                # If it\u0027s the final chunk, we\u0027re in (possibly empty) trailers"}],"source_content_type":"text/x-python","patch_set":2,"id":"2ccc9b7f_1a878d9f","line":434,"updated":"2026-05-27 18:57:18.000000000","message":"Well that\u0027s what I get for thinking the reader would eventually raise some kind of `IOError` or something...","commit_id":"5ad23e2f6b91f75a8fe0008c8dfc9fb3d4bb6ca3"}],"test/unit/common/middleware/s3api/test_obj.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ee70e23aedc32f2160c42af7d92addd371cb615d","unresolved":true,"context_lines":[{"line_number":851,"context_line":"        try:"},{"line_number":852,"context_line":"            resp \u003d req.get_response(self.s3api)"},{"line_number":853,"context_line":"        except EndOfLine:"},{"line_number":854,"context_line":"            self.fail(\u0027read after eof\u0027)"},{"line_number":855,"context_line":""},{"line_number":856,"context_line":"        self.assertEqual(400, resp.status_int)"},{"line_number":857,"context_line":"        self.assertEqual(\u0027IncompleteBody\u0027, self._get_error_code(resp.body))"}],"source_content_type":"text/x-python","patch_set":1,"id":"3e40cd3b_0b8da070","line":854,"updated":"2026-05-11 17:08:15.000000000","message":"ok, I see this fire when I revert the fix","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"}],"test/unit/common/middleware/s3api/test_s3request.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ee70e23aedc32f2160c42af7d92addd371cb615d","unresolved":true,"context_lines":[{"line_number":1808,"context_line":"               \u0027a\\r\\nklm\u0027"},{"line_number":1809,"context_line":"        s3req \u003d self._test_sig_v4_streaming_unsigned_payload_trailer(body)"},{"line_number":1810,"context_line":"        with self.assertRaises(s3request.S3InputIncomplete):"},{"line_number":1811,"context_line":"            s3req.environ[\u0027wsgi.input\u0027].read()"},{"line_number":1812,"context_line":""},{"line_number":1813,"context_line":"    def test_sig_v4_strm_unsgnd_pyld_trl_none_ok(self):"},{"line_number":1814,"context_line":"        # verify it\u0027s ok to not send any trailer"}],"source_content_type":"text/x-python","patch_set":1,"id":"b23ba031_f5622497","line":1811,"updated":"2026-05-11 17:08:15.000000000","message":"this doesn\u0027t complete if I revert the change","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"bf459fc8d6f77017f793fcd5205aa22c1a8fff0c","unresolved":true,"context_lines":[{"line_number":1808,"context_line":"               \u0027a\\r\\nklm\u0027"},{"line_number":1809,"context_line":"        s3req \u003d self._test_sig_v4_streaming_unsigned_payload_trailer(body)"},{"line_number":1810,"context_line":"        with self.assertRaises(s3request.S3InputIncomplete):"},{"line_number":1811,"context_line":"            s3req.environ[\u0027wsgi.input\u0027].read()"},{"line_number":1812,"context_line":""},{"line_number":1813,"context_line":"    def test_sig_v4_strm_unsgnd_pyld_trl_none_ok(self):"},{"line_number":1814,"context_line":"        # verify it\u0027s ok to not send any trailer"}],"source_content_type":"text/x-python","patch_set":1,"id":"de0996a2_62ed48e5","line":1811,"in_reply_to":"b23ba031_f5622497","updated":"2026-05-12 04:12:17.000000000","message":"yeah, this test case would hang on the master branch.\n\nthis new regression test at ``StreamingInput`` scope would see AssertionError on master: https://review.opendev.org/c/openstack/swift/+/988199\n\nsince the fix is within ``StreamingInput``, a test at same scope would also be better. look at the its neighbor test cases, ``test_chunk_separator_missing`` and ``test_final_newline_missing``, the new ``test_client_disconnect_mid_chunk`` would be a good addition?","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"}]}