)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"ef000820_9ebc8bbb","updated":"2023-10-09 17:46:19.000000000","message":"LGTM\n\nThere\u0027s a gap in the (existing) tests for drain_and_close: https://review.opendev.org/c/openstack/swift/+/897709","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6d9c17781b28cbc93e4818ef5fd33aec8fc019bc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"03b99ce4_beb00b3d","updated":"2023-10-10 00:38:44.000000000","message":"I think I noticed that many of the new conditional tests only had to add expected_unread assertions AFTER the refactor; so there may have been a leak introduced there!\n\nThis change removes the introduction friendly_close onto the exsting implementation; it would be trivial to add it back in - but I think I\u0027d like to better understand the existing behavior better across more test scenarios before we start changing it.\n\nIt would be totally fine with me if slo doesn\u0027t actually use friendly_close (or whatever we call it) until the refactor - at then that change could have many \"XXX why doesn\u0027t this get closed\" removed.  Back porting the \"fix\" into the existing implementation isn\u0027t a high priority for me.\n\nIf we got that route I\u0027m not sure which patch should introduce friendly_close - presumably the refactor?  I want the test leak tracking to go into the test refactor as I definately don\u0027t want the refactor to have any regressions on closing manifest sub-requests.","commit_id":"800578c71bcfefaebbc39bb763dc9a0b43e5870c"}],"swift/common/middleware/slo.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":830,"context_line":"            return resp_iter"},{"line_number":831,"context_line":""},{"line_number":832,"context_line":"        is_conditional \u003d self._response_status.startswith((\u0027304\u0027, \u0027412\u0027)) and ("},{"line_number":833,"context_line":"            req.if_match or req.if_none_match)"},{"line_number":834,"context_line":"        if slo_etag and slo_size and ("},{"line_number":835,"context_line":"                req.method \u003d\u003d \u0027HEAD\u0027 or is_conditional):"},{"line_number":836,"context_line":"            # swob is going to throw away this body anyway"}],"source_content_type":"text/x-python","patch_set":3,"id":"097c4f78_df780144","line":833,"range":{"start_line":833,"start_character":12,"end_line":833,"end_character":45},"updated":"2023-10-09 17:46:19.000000000","message":"off-topic: why doesn\u0027t this condition include if_(un)modified_since?\n\nlooks like in the next patch (refactor) the req.if_(none_)match part of the condition is dropped :)","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":834,"context_line":"        if slo_etag and slo_size and ("},{"line_number":835,"context_line":"                req.method \u003d\u003d \u0027HEAD\u0027 or is_conditional):"},{"line_number":836,"context_line":"            # swob is going to throw away this body anyway"},{"line_number":837,"context_line":"            friendly_close(resp_iter)"},{"line_number":838,"context_line":"            # Since we have length and etag, we can respond immediately"},{"line_number":839,"context_line":"            resp \u003d Response("},{"line_number":840,"context_line":"                status\u003dself._response_status,"}],"source_content_type":"text/x-python","patch_set":3,"id":"e0a32d69_dcf502eb","line":837,"updated":"2023-10-09 17:46:19.000000000","message":"ok. backend response is a manifest or 304 or 412 which swob ensures won\u0027t have a body. reasonable to drain a little and close.","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":853,"context_line":""},{"line_number":854,"context_line":"        if self._need_to_refetch_manifest(req):"},{"line_number":855,"context_line":"            req.environ[\u0027swift.non_client_disconnect\u0027] \u003d True"},{"line_number":856,"context_line":"            friendly_close(resp_iter)"},{"line_number":857,"context_line":"            del req.environ[\u0027swift.non_client_disconnect\u0027]"},{"line_number":858,"context_line":""},{"line_number":859,"context_line":"            get_req \u003d make_subrequest("}],"source_content_type":"text/x-python","patch_set":3,"id":"b6786297_3f985608","line":856,"updated":"2023-10-09 17:46:19.000000000","message":"if we get here, we think original response is an slo manifest, so reasonable to drain a little","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"}],"swift/common/swob.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":1392,"context_line":"    def _response_iter(self, app_iter, body):"},{"line_number":1393,"context_line":"        if self.conditional_response and self.request:"},{"line_number":1394,"context_line":"            empty_resp \u003d self._get_conditional_response_status()"},{"line_number":1395,"context_line":"            if empty_resp is not None:"},{"line_number":1396,"context_line":"                self.status \u003d empty_resp"},{"line_number":1397,"context_line":"                self.content_length \u003d 0"},{"line_number":1398,"context_line":"                # it\u0027s not obviously reasonable to use friendly_close here, in"}],"source_content_type":"text/x-python","patch_set":3,"id":"bb3894a3_4eb01747","line":1395,"updated":"2023-10-09 17:46:19.000000000","message":"this could also be a HEAD","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":1403,"context_line":""},{"line_number":1404,"context_line":"        if self.request and self.request.method \u003d\u003d \u0027HEAD\u0027:"},{"line_number":1405,"context_line":"            # We explicitly do NOT want to set self.content_length to 0 here"},{"line_number":1406,"context_line":"            friendly_close(app_iter)  # be friendly to our app_iter"},{"line_number":1407,"context_line":"            return [b\u0027\u0027]"},{"line_number":1408,"context_line":""},{"line_number":1409,"context_line":"        if self.conditional_response and self.request and \\"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f708c39_8aad1b1d","line":1406,"updated":"2023-10-09 17:46:19.000000000","message":"yes, obviously reasonable","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":1412,"context_line":"            ranges \u003d self.request.range.ranges_for_length(self.content_length)"},{"line_number":1413,"context_line":"            if ranges \u003d\u003d []:"},{"line_number":1414,"context_line":"                self.status \u003d 416"},{"line_number":1415,"context_line":"                close_if_possible(app_iter)"},{"line_number":1416,"context_line":"                self.headers[\u0027Content-Range\u0027] \u003d \\"},{"line_number":1417,"context_line":"                    \u0027bytes */%d\u0027 % self.content_length"},{"line_number":1418,"context_line":"                # Setting body + app_iter to None makes us emit the default"}],"source_content_type":"text/x-python","patch_set":3,"id":"794a0f7b_347631b9","line":1415,"updated":"2023-10-09 17:46:19.000000000","message":"not reasonable to be drain here for same reason","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"}],"swift/common/utils/__init__.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":4037,"context_line":"    close_if_possible(app_iter)"},{"line_number":4038,"context_line":""},{"line_number":4039,"context_line":""},{"line_number":4040,"context_line":"def friendly_close(resp):"},{"line_number":4041,"context_line":"    \"\"\""},{"line_number":4042,"context_line":"    Close a swob or WSGI response and maybe drain it."},{"line_number":4043,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"351fa6d3_b0c8e721","line":4040,"updated":"2023-10-09 17:46:19.000000000","message":"nit: (naming :/) I like that drain_and_close does what it says, whereas friendly_close loses the mention of drain. limited_drain_and_close? But it\u0027s a nit. really.","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":4045,"context_line":"    are probably already in our network buffers.  For a larger response we"},{"line_number":4046,"context_line":"    could possibly burn a lot of CPU/network trying to drain an un-used"},{"line_number":4047,"context_line":"    response.  This method will read up to DEFAULT_DRAIN_LIMIT bytes to avoid"},{"line_number":4048,"context_line":"    logging a 499 in the proxy when it would otherwise be easy to just throw"},{"line_number":4049,"context_line":"    away the small/empty body."},{"line_number":4050,"context_line":"    \"\"\""},{"line_number":4051,"context_line":"    return drain_and_close(resp, read_limit\u003dDEFAULT_DRAIN_LIMIT)"}],"source_content_type":"text/x-python","patch_set":3,"id":"028269d8_0e835101","line":4048,"range":{"start_line":4048,"start_character":4,"end_line":4048,"end_character":30},"updated":"2023-10-09 17:46:19.000000000","message":"do we have any test that would validate this?","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"}],"test/unit/common/middleware/test_slo.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":1876,"context_line":"        # SloTestCase always has an app"},{"line_number":1877,"context_line":"        self.assertEqual(self.app.unclosed_requests, {})"},{"line_number":1878,"context_line":"        self.assertEqual(self.app.unread_requests,"},{"line_number":1879,"context_line":"                         self.expected_unread_requests)"},{"line_number":1880,"context_line":""},{"line_number":1881,"context_line":"    def _setup_manifest_bc(self):"},{"line_number":1882,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"f14530c5_8c589156","line":1879,"updated":"2023-10-09 17:46:19.000000000","message":"off-topic: it would be prudent to call super(SloGETorHEADTestCase, self).tearDown() in case it is ever implemented in SloTestCase","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"}],"test/unit/common/test_utils.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1b89b5c0cef27b5a19fc91ac4c029c37004aac12","unresolved":true,"context_lines":[{"line_number":550,"context_line":"        utils.drain_and_close(Response(status\u003d200, body\u003db\u0027Some body\u0027))"},{"line_number":551,"context_line":"        drained \u003d [False]"},{"line_number":552,"context_line":"        utils.drain_and_close(Response(status\u003d200, app_iter\u003dgen()))"},{"line_number":553,"context_line":"        self.assertTrue(drained[0])"},{"line_number":554,"context_line":""},{"line_number":555,"context_line":"    def test_drain_and_close_with_limit(self):"},{"line_number":556,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"6e5128ae_b0b2008c","line":553,"updated":"2023-10-09 17:46:19.000000000","message":"we don\u0027t assert that the generator is closed","commit_id":"17b67ed339662fc92b31e4faf1db271b8d5ad9d9"}]}