From 683abdfe8a109a8fa419c6c3e46b98a949c402ad Mon Sep 17 00:00:00 2001 From: HemangChothani Date: Thu, 12 Dec 2019 15:24:17 +0530 Subject: [PATCH 1/2] fix(storage): fix timeout error in do_request and unit test --- storage/google/cloud/storage/batch.py | 9 ++++++- storage/tests/unit/test__http.py | 6 ++++- storage/tests/unit/test_batch.py | 10 +++---- storage/tests/unit/test_client.py | 38 +++++++++++++-------------- 4 files changed, 37 insertions(+), 26 deletions(-) diff --git a/storage/google/cloud/storage/batch.py b/storage/google/cloud/storage/batch.py index 772531222e24..cf31c3483b73 100644 --- a/storage/google/cloud/storage/batch.py +++ b/storage/google/cloud/storage/batch.py @@ -150,7 +150,7 @@ def __init__(self, client): self._requests = [] self._target_objects = [] - def _do_request(self, method, url, headers, data, target_object): + def _do_request(self, method, url, headers, data, target_object, timeout): """Override Connection: defer actual HTTP request. Only allow up to ``_MAX_BATCH_SIZE`` requests to be deferred. @@ -173,6 +173,13 @@ def _do_request(self, method, url, headers, data, target_object): connection. Here we defer an HTTP request and complete initialization of the object at a later time. + :type timeout: float or tuple + :param timeout: (optional) The amount of time, in seconds, to wait + for the server response. By default, the method waits indefinitely. + + Can also be passed as a tuple (connect_timeout, read_timeout). + See :meth:`requests.Session.request` documentation for details. + :rtype: tuple of ``response`` (a dictionary of sorts) and ``content`` (a string). :returns: The HTTP response object and the content of the response. diff --git a/storage/tests/unit/test__http.py b/storage/tests/unit/test__http.py index 5f61ea2d7a0b..3fefaf6bcbd5 100644 --- a/storage/tests/unit/test__http.py +++ b/storage/tests/unit/test__http.py @@ -51,7 +51,11 @@ def test_extra_headers(self): } expected_uri = conn.build_api_url("/rainbow") http.request.assert_called_once_with( - data=req_data, headers=expected_headers, method="GET", url=expected_uri + data=req_data, + headers=expected_headers, + method="GET", + timeout=None, + url=expected_uri, ) def test_build_api_url_no_extra_query_params(self): diff --git a/storage/tests/unit/test_batch.py b/storage/tests/unit/test_batch.py index 1c95807f0a22..488c4c4b31ac 100644 --- a/storage/tests/unit/test_batch.py +++ b/storage/tests/unit/test_batch.py @@ -314,9 +314,9 @@ def test_finish_nonempty(self): batch = self._make_one(client) batch.API_BASE_URL = "http://api.example.com" - batch._do_request("POST", url, {}, {"foo": 1, "bar": 2}, None) - batch._do_request("PATCH", url, {}, {"bar": 3}, None) - batch._do_request("DELETE", url, {}, None, None) + batch._do_request("POST", url, {}, {"foo": 1, "bar": 2}, None, None) + batch._do_request("PATCH", url, {}, {"bar": 3}, None, None) + batch._do_request("DELETE", url, {}, None, None, None) result = batch.finish() self.assertEqual(len(result), len(batch._requests)) @@ -389,8 +389,8 @@ def test_finish_nonempty_with_status_failure(self): target1 = _MockObject() target2 = _MockObject() - batch._do_request("GET", url, {}, None, target1) - batch._do_request("GET", url, {}, None, target2) + batch._do_request("GET", url, {}, None, target1, None) + batch._do_request("GET", url, {}, None, target2, None) # Make sure futures are not populated. self.assertEqual( diff --git a/storage/tests/unit/test_client.py b/storage/tests/unit/test_client.py index 36bf3d97cb65..cbf27d88ebe7 100644 --- a/storage/tests/unit/test_client.py +++ b/storage/tests/unit/test_client.py @@ -271,7 +271,7 @@ def test_get_service_account_email_wo_project(self): ] ) http.request.assert_called_once_with( - method="GET", url=URI, data=None, headers=mock.ANY + method="GET", url=URI, data=None, headers=mock.ANY, timeout=None ) def test_get_service_account_email_w_project(self): @@ -297,7 +297,7 @@ def test_get_service_account_email_w_project(self): ] ) http.request.assert_called_once_with( - method="GET", url=URI, data=None, headers=mock.ANY + method="GET", url=URI, data=None, headers=mock.ANY, timeout=None ) def test_bucket(self): @@ -366,7 +366,7 @@ def test_get_bucket_with_string_miss(self): client.get_bucket(NONESUCH) http.request.assert_called_once_with( - method="GET", url=URI, data=mock.ANY, headers=mock.ANY + method="GET", url=URI, data=mock.ANY, headers=mock.ANY, timeout=None ) def test_get_bucket_with_string_hit(self): @@ -396,7 +396,7 @@ def test_get_bucket_with_string_hit(self): self.assertIsInstance(bucket, Bucket) self.assertEqual(bucket.name, BUCKET_NAME) http.request.assert_called_once_with( - method="GET", url=URI, data=mock.ANY, headers=mock.ANY + method="GET", url=URI, data=mock.ANY, headers=mock.ANY, timeout=None ) def test_get_bucket_with_object_miss(self): @@ -427,7 +427,7 @@ def test_get_bucket_with_object_miss(self): client.get_bucket(bucket_obj) http.request.assert_called_once_with( - method="GET", url=URI, data=mock.ANY, headers=mock.ANY + method="GET", url=URI, data=mock.ANY, headers=mock.ANY, timeout=None ) def test_get_bucket_with_object_hit(self): @@ -458,7 +458,7 @@ def test_get_bucket_with_object_hit(self): self.assertIsInstance(bucket, Bucket) self.assertEqual(bucket.name, bucket_name) http.request.assert_called_once_with( - method="GET", url=URI, data=mock.ANY, headers=mock.ANY + method="GET", url=URI, data=mock.ANY, headers=mock.ANY, timeout=None ) def test_lookup_bucket_miss(self): @@ -485,7 +485,7 @@ def test_lookup_bucket_miss(self): self.assertIsNone(bucket) http.request.assert_called_once_with( - method="GET", url=URI, data=mock.ANY, headers=mock.ANY + method="GET", url=URI, data=mock.ANY, headers=mock.ANY, timeout=None ) def test_lookup_bucket_hit(self): @@ -514,7 +514,7 @@ def test_lookup_bucket_hit(self): self.assertIsInstance(bucket, Bucket) self.assertEqual(bucket.name, BUCKET_NAME) http.request.assert_called_once_with( - method="GET", url=URI, data=mock.ANY, headers=mock.ANY + method="GET", url=URI, data=mock.ANY, headers=mock.ANY, timeout=None ) def test_create_bucket_w_missing_client_project(self): @@ -666,7 +666,7 @@ def test_create_bucket_w_string_success(self): self.assertEqual(bucket.name, bucket_name) self.assertTrue(bucket.requester_pays) http.request.assert_called_once_with( - method="POST", url=URI, data=mock.ANY, headers=mock.ANY + method="POST", url=URI, data=mock.ANY, headers=mock.ANY, timeout=None ) json_sent = http.request.call_args_list[0][1]["data"] self.assertEqual(json_expected, json.loads(json_sent)) @@ -706,7 +706,7 @@ def test_create_bucket_w_object_success(self): self.assertEqual(bucket.name, bucket_name) self.assertTrue(bucket.requester_pays) http.request.assert_called_once_with( - method="POST", url=URI, data=mock.ANY, headers=mock.ANY + method="POST", url=URI, data=mock.ANY, headers=mock.ANY, timeout=None ) json_sent = http.request.call_args_list[0][1]["data"] self.assertEqual(json_expected, json.loads(json_sent)) @@ -848,7 +848,7 @@ def test_list_buckets_empty(self): self.assertEqual(len(buckets), 0) http.request.assert_called_once_with( - method="GET", url=mock.ANY, data=mock.ANY, headers=mock.ANY + method="GET", url=mock.ANY, data=mock.ANY, headers=mock.ANY, timeout=None ) requested_url = http.request.mock_calls[0][2]["url"] @@ -883,7 +883,7 @@ def test_list_buckets_explicit_project(self): self.assertEqual(len(buckets), 0) http.request.assert_called_once_with( - method="GET", url=mock.ANY, data=mock.ANY, headers=mock.ANY + method="GET", url=mock.ANY, data=mock.ANY, headers=mock.ANY, timeout=None ) requested_url = http.request.mock_calls[0][2]["url"] @@ -918,7 +918,7 @@ def test_list_buckets_non_empty(self): self.assertEqual(buckets[0].name, BUCKET_NAME) http.request.assert_called_once_with( - method="GET", url=mock.ANY, data=mock.ANY, headers=mock.ANY + method="GET", url=mock.ANY, data=mock.ANY, headers=mock.ANY, timeout=None ) def test_list_buckets_all_arguments(self): @@ -948,7 +948,7 @@ def test_list_buckets_all_arguments(self): buckets = list(iterator) self.assertEqual(buckets, []) http.request.assert_called_once_with( - method="GET", url=mock.ANY, data=mock.ANY, headers=mock.ANY + method="GET", url=mock.ANY, data=mock.ANY, headers=mock.ANY, timeout=None ) requested_url = http.request.mock_calls[0][2]["url"] @@ -1077,7 +1077,7 @@ def _create_hmac_key_helper(self, explicit_project=None, user_project=None): FULL_URI = "{}?{}".format(URI, urlencode(qs_params)) http.request.assert_called_once_with( - method="POST", url=FULL_URI, data=None, headers=mock.ANY + method="POST", url=FULL_URI, data=None, headers=mock.ANY, timeout=None ) def test_create_hmac_key_defaults(self): @@ -1112,7 +1112,7 @@ def test_list_hmac_keys_defaults_empty(self): ] ) http.request.assert_called_once_with( - method="GET", url=URI, data=None, headers=mock.ANY + method="GET", url=URI, data=None, headers=mock.ANY, timeout=None ) def test_list_hmac_keys_explicit_non_empty(self): @@ -1176,7 +1176,7 @@ def test_list_hmac_keys_explicit_non_empty(self): "userProject": USER_PROJECT, } http.request.assert_called_once_with( - method="GET", url=mock.ANY, data=None, headers=mock.ANY + method="GET", url=mock.ANY, data=None, headers=mock.ANY, timeout=None ) kwargs = http.request.mock_calls[0].kwargs uri = kwargs["url"] @@ -1223,7 +1223,7 @@ def test_get_hmac_key_metadata_wo_project(self): ] ) http.request.assert_called_once_with( - method="GET", url=URI, data=None, headers=mock.ANY + method="GET", url=URI, data=None, headers=mock.ANY, timeout=None ) def test_get_hmac_key_metadata_w_project(self): @@ -1273,5 +1273,5 @@ def test_get_hmac_key_metadata_w_project(self): FULL_URI = "{}?{}".format(URI, urlencode(qs_params)) http.request.assert_called_once_with( - method="GET", url=FULL_URI, data=None, headers=mock.ANY + method="GET", url=FULL_URI, data=None, headers=mock.ANY, timeout=None ) From f78a94ab39aee0613cd73aef163954bac113dbed Mon Sep 17 00:00:00 2001 From: HemangChothani Date: Wed, 18 Dec 2019 14:43:34 +0530 Subject: [PATCH 2/2] fix(storage): Implement timeout and testcase changes --- storage/google/cloud/storage/batch.py | 13 +++---- storage/tests/unit/test_batch.py | 50 ++++++++++++++++++--------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/storage/google/cloud/storage/batch.py b/storage/google/cloud/storage/batch.py index cf31c3483b73..c1dd0348a8f8 100644 --- a/storage/google/cloud/storage/batch.py +++ b/storage/google/cloud/storage/batch.py @@ -150,7 +150,7 @@ def __init__(self, client): self._requests = [] self._target_objects = [] - def _do_request(self, method, url, headers, data, target_object, timeout): + def _do_request(self, method, url, headers, data, target_object, timeout=None): """Override Connection: defer actual HTTP request. Only allow up to ``_MAX_BATCH_SIZE`` requests to be deferred. @@ -188,7 +188,7 @@ def _do_request(self, method, url, headers, data, target_object, timeout): raise ValueError( "Too many deferred requests (max %d)" % self._MAX_BATCH_SIZE ) - self._requests.append((method, url, headers, data)) + self._requests.append((method, url, headers, data, timeout)) result = _FutureDict() self._target_objects.append(target_object) if target_object is not None: @@ -207,7 +207,7 @@ def _prepare_batch_request(self): multi = MIMEMultipart() - for method, uri, headers, body in self._requests: + for method, uri, headers, body, timeout in self._requests: subrequest = MIMEApplicationHTTP(method, uri, headers, body) multi.attach(subrequest) @@ -222,7 +222,7 @@ def _prepare_batch_request(self): # Strip off redundant header text _, body = payload.split("\n\n", 1) - return dict(multi._headers), body + return dict(multi._headers), body, timeout def _finish_futures(self, responses): """Apply all the batch responses to the futures created. @@ -258,15 +258,16 @@ def finish(self): :rtype: list of tuples :returns: one ``(headers, payload)`` tuple per deferred request. """ - headers, body = self._prepare_batch_request() + headers, body, timeout = self._prepare_batch_request() url = "%s/batch/storage/v1" % self.API_BASE_URL # Use the private ``_base_connection`` rather than the property # ``_connection``, since the property may be this # current batch. + response = self._client._base_connection._make_request( - "POST", url, data=body, headers=headers + "POST", url, data=body, headers=headers, timeout=timeout ) responses = list(_unpack_batch_response(response)) self._finish_futures(responses) diff --git a/storage/tests/unit/test_batch.py b/storage/tests/unit/test_batch.py index 488c4c4b31ac..da60811d8186 100644 --- a/storage/tests/unit/test_batch.py +++ b/storage/tests/unit/test_batch.py @@ -133,7 +133,7 @@ def test__make_request_GET_normal(self): batch = self._make_one(connection) target = _MockObject() - response = batch._make_request("GET", url, target_object=target) + response = batch._make_request("GET", url, target_object=target, timeout=None) # Check the respone self.assertEqual(response.status_code, 204) @@ -147,10 +147,11 @@ def test__make_request_GET_normal(self): # Check the queued request self.assertEqual(len(batch._requests), 1) request = batch._requests[0] - request_method, request_url, _, request_data = request + request_method, request_url, _, request_data, request_timeout = request self.assertEqual(request_method, "GET") self.assertEqual(request_url, url) self.assertIsNone(request_data) + self.assertIsNone(request_timeout) def test__make_request_POST_normal(self): from google.cloud.storage.batch import _FutureDict @@ -163,7 +164,7 @@ def test__make_request_POST_normal(self): target = _MockObject() response = batch._make_request( - "POST", url, data={"foo": 1}, target_object=target + "POST", url, data={"foo": 1}, target_object=target, timeout=None ) self.assertEqual(response.status_code, 204) @@ -174,10 +175,11 @@ def test__make_request_POST_normal(self): http.request.assert_not_called() request = batch._requests[0] - request_method, request_url, _, request_data = request + request_method, request_url, _, request_data, request_timeout = request self.assertEqual(request_method, "POST") self.assertEqual(request_url, url) self.assertEqual(request_data, data) + self.assertIsNone(request_timeout) def test__make_request_PATCH_normal(self): from google.cloud.storage.batch import _FutureDict @@ -190,7 +192,7 @@ def test__make_request_PATCH_normal(self): target = _MockObject() response = batch._make_request( - "PATCH", url, data={"foo": 1}, target_object=target + "PATCH", url, data={"foo": 1}, target_object=target, timeout=None ) self.assertEqual(response.status_code, 204) @@ -201,10 +203,11 @@ def test__make_request_PATCH_normal(self): http.request.assert_not_called() request = batch._requests[0] - request_method, request_url, _, request_data = request + request_method, request_url, _, request_data, request_timeout = request self.assertEqual(request_method, "PATCH") self.assertEqual(request_url, url) self.assertEqual(request_data, data) + self.assertIsNone(request_timeout) def test__make_request_DELETE_normal(self): from google.cloud.storage.batch import _FutureDict @@ -214,8 +217,11 @@ def test__make_request_DELETE_normal(self): connection = _Connection(http=http) batch = self._make_one(connection) target = _MockObject() + timeout = 1 - response = batch._make_request("DELETE", url, target_object=target) + response = batch._make_request( + "DELETE", url, target_object=target, timeout=timeout + ) # Check the respone self.assertEqual(response.status_code, 204) @@ -228,22 +234,22 @@ def test__make_request_DELETE_normal(self): # Check the queued request self.assertEqual(len(batch._requests), 1) request = batch._requests[0] - request_method, request_url, _, request_data = request + request_method, request_url, _, request_data, request_timeout = request self.assertEqual(request_method, "DELETE") self.assertEqual(request_url, url) self.assertIsNone(request_data) + self.assertEqual(request_timeout, timeout) def test__make_request_POST_too_many_requests(self): url = "http://example.com/api" http = _make_requests_session([]) connection = _Connection(http=http) batch = self._make_one(connection) - batch._MAX_BATCH_SIZE = 1 - batch._requests.append(("POST", url, {}, {"bar": 2})) + batch._requests.append(("POST", url, {}, {"bar": 2}, 1)) with self.assertRaises(ValueError): - batch._make_request("POST", url, data={"foo": 1}) + batch._make_request("POST", url, data={"foo": 1}, timeout=1) def test_finish_empty(self): http = _make_requests_session([]) @@ -340,7 +346,11 @@ def test_finish_nonempty(self): expected_url = "{}/batch/storage/v1".format(batch.API_BASE_URL) http.request.assert_called_once_with( - method="POST", url=expected_url, headers=mock.ANY, data=mock.ANY + method="POST", + url=expected_url, + headers=mock.ANY, + data=mock.ANY, + timeout=mock.ANY, ) request_info = self._get_mutlipart_request(http) @@ -369,7 +379,7 @@ def test_finish_responses_mismatch(self): batch = self._make_one(client) batch.API_BASE_URL = "http://api.example.com" - batch._requests.append(("GET", url, {}, None)) + batch._requests.append(("GET", url, {}, None, 1)) with self.assertRaises(ValueError): batch.finish() @@ -406,7 +416,11 @@ def test_finish_nonempty_with_status_failure(self): expected_url = "{}/batch/storage/v1".format(batch.API_BASE_URL) http.request.assert_called_once_with( - method="POST", url=expected_url, headers=mock.ANY, data=mock.ANY + method="POST", + url=expected_url, + headers=mock.ANY, + data=mock.ANY, + timeout=mock.ANY, ) _, request_body, _, boundary = self._get_mutlipart_request(http) @@ -422,7 +436,7 @@ def test_finish_nonempty_non_multipart_response(self): connection = _Connection(http=http) client = _Client(connection) batch = self._make_one(client) - batch._requests.append(("POST", url, {}, {"foo": 1, "bar": 2})) + batch._requests.append(("POST", url, {}, {"foo": 1, "bar": 2}, 1)) with self.assertRaises(ValueError): batch.finish() @@ -620,8 +634,10 @@ class _Connection(object): def __init__(self, **kw): self.__dict__.update(kw) - def _make_request(self, method, url, data=None, headers=None): - return self.http.request(url=url, method=method, headers=headers, data=data) + def _make_request(self, method, url, data=None, headers=None, timeout=None): + return self.http.request( + url=url, method=method, headers=headers, data=data, timeout=timeout + ) class _MockObject(object):