Skip to content

Commit

Permalink
(BSR)[API] fix: Fix non-thread safe usage of boto3 client in outscale…
Browse files Browse the repository at this point in the history
… connector

`boto3.client()` is not thread-safe. We sometimes get the following
error:

    KeyError: 'default_config_resolver'
      File "pcapi/tasks/ubble_tasks.py", line 23, in store_id_pictures_task
        ubble_subscription_api.archive_ubble_user_id_pictures(payload.identification_id)
      [...]
      File "pcapi/connectors/beneficiaries/outscale.py", line 13, in upload_file
        client = boto3.client(
      File "__init__.py", line 92, in client
        return _get_default_session().client(*args, **kwargs)
      [...]
      File "botocore/session.py", line 1009, in get_component
        del self._deferred[name]

To work around that, we now use a dedicated session. And we keep one
for each thread to lessen the overhead of the session initialization.

References:
 - discussion of the issue and possible workarounds: boto/boto3#1592
 - similar fix in Sentry client code: getsentry/sentry@f58ca5e
  • Loading branch information
dbaty committed Apr 4, 2023
1 parent bce1bd1 commit 84ae82a
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 16 deletions.
15 changes: 14 additions & 1 deletion api/src/pcapi/connectors/beneficiaries/outscale.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
import threading

import boto3
from botocore.exceptions import ClientError
Expand All @@ -8,9 +9,21 @@

logger = logging.getLogger(__name__)

_thread_local_boto_session_container = threading.local()


def _get_boto_session() -> boto3.Session:
if not hasattr(_thread_local_boto_session_container, "session"):
_thread_local_boto_session_container.session = boto3.Session()
return _thread_local_boto_session_container.session


def upload_file(user_id: str, file_path: str, file_name: str) -> bool:
client = boto3.client(
# Do not use `boto3.client()`, it is not thread safe. Instead, get
# a session (and keep it for the whole duration of this thread,
# since it's a bit costly to create).
session = _get_boto_session()
client = session.client(
"s3",
aws_access_key_id=settings.OUTSCALE_ACCESS_KEY,
aws_secret_access_key=settings.OUTSCALE_SECRET_KEY,
Expand Down
6 changes: 3 additions & 3 deletions api/tests/connectors/beneficiaries/outscale_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ def test_upload_file_not_found(self, caplog) -> None:
record = caplog.records[0]
assert expected_log_message in record.message

@patch("pcapi.connectors.beneficiaries.outscale.boto3.client")
def test_upload_file_succesfully(self, mock_s3_client, caplog) -> None:
@patch("botocore.session.Session.create_client")
def test_upload_file_succesfully(self, mocked_storage_client, caplog) -> None:
# Given
file_name = "carte_identite_front.png"
file_path = f"{IMAGES_DIR}/{file_name}"
Expand All @@ -41,7 +41,7 @@ def test_upload_file_succesfully(self, mock_s3_client, caplog) -> None:
outscale.upload_file(user_id, file_path, file_name)

# Then
assert mock_s3_client.return_value.upload_file.call_count == 1
assert mocked_storage_client.return_value.upload_file.call_count == 1
assert len(caplog.records) >= 1
record = caplog.records[0]
assert expected_log_message in record.message
Expand Down
26 changes: 14 additions & 12 deletions api/tests/core/subscription/ubble/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -519,8 +519,8 @@ def test_download_ubble_document_pictures_with_unknown_error(self, requests_mock
assert self.picture_path in record.extra["url"]
assert record.extra["status_code"] == 503

@patch("pcapi.connectors.beneficiaries.outscale.boto3.client")
def test_download_ubble_document_pictures_successfully(self, mock_s3_client, requests_mock):
@patch("botocore.session.Session.create_client")
def test_download_ubble_document_pictures_successfully(self, mocked_storage_client, requests_mock):
# Given
with open(f"{IMAGES_DIR}/carte_identite_front.png", "rb") as img:
identity_file_picture = BytesIO(img.read())
Expand All @@ -532,7 +532,7 @@ def test_download_ubble_document_pictures_successfully(self, mock_s3_client, req
# When
ubble_subscription_api._download_and_store_ubble_picture(self.fraud_check, self.picture_path, "front")

assert mock_s3_client.return_value.upload_file.call_count == 1
assert mocked_storage_client.return_value.upload_file.call_count == 1


@pytest.mark.usefixtures("db_session")
Expand Down Expand Up @@ -599,8 +599,8 @@ def test_archive_ubble_user_id_pictures_no_file_saved(self, ubble_mocker, reques
db.session.refresh(fraud_check)
assert fraud_check.idPicturesStored is False

@patch("pcapi.connectors.beneficiaries.outscale.boto3.client")
def test_archive_ubble_user_id_pictures_only_front_saved(self, mock_s3_client, ubble_mocker, requests_mock):
@patch("botocore.session.Session.create_client")
def test_archive_ubble_user_id_pictures_only_front_saved(self, mocked_storage_client, ubble_mocker, requests_mock):
# Given
fraud_check = BeneficiaryFraudCheckFactory(status=FraudCheckStatus.OK, type=FraudCheckType.UBBLE)

Expand Down Expand Up @@ -638,10 +638,10 @@ def test_archive_ubble_user_id_pictures_only_front_saved(self, mock_s3_client, u
# Then
assert exc_info.value.is_retryable is True
assert fraud_check.idPicturesStored is False
assert mock_s3_client.return_value.upload_file.call_count == 1
assert mocked_storage_client.return_value.upload_file.call_count == 1

@patch("pcapi.connectors.beneficiaries.outscale.boto3.client")
def test_archive_ubble_user_id_pictures_both_files_saved(self, mock_s3_client, ubble_mocker, requests_mock):
@patch("botocore.session.Session.create_client")
def test_archive_ubble_user_id_pictures_both_files_saved(self, mocked_storage_client, ubble_mocker, requests_mock):
# Given
fraud_check = BeneficiaryFraudCheckFactory(status=FraudCheckStatus.OK, type=FraudCheckType.UBBLE)

Expand Down Expand Up @@ -689,10 +689,12 @@ def test_archive_ubble_user_id_pictures_both_files_saved(self, mock_s3_client, u
# Then
db.session.refresh(fraud_check)
assert fraud_check.idPicturesStored is expected_id_pictures_stored
assert mock_s3_client.return_value.upload_file.call_count == 2
assert mocked_storage_client.return_value.upload_file.call_count == 2

@patch("pcapi.connectors.beneficiaries.outscale.boto3.client")
def test_archive_ubble_user_id_pictures_all_expected_files_saved(self, mock_s3_client, ubble_mocker, requests_mock):
@patch("botocore.session.Session.create_client")
def test_archive_ubble_user_id_pictures_all_expected_files_saved(
self, mocked_storage_client, ubble_mocker, requests_mock
):
# Given
fraud_check = BeneficiaryFraudCheckFactory(status=FraudCheckStatus.OK, type=FraudCheckType.UBBLE)

Expand Down Expand Up @@ -732,7 +734,7 @@ def test_archive_ubble_user_id_pictures_all_expected_files_saved(self, mock_s3_c
# Then
db.session.refresh(fraud_check)
assert fraud_check.idPicturesStored is expected_id_pictures_stored
assert mock_s3_client.return_value.upload_file.call_count == 1
assert mocked_storage_client.return_value.upload_file.call_count == 1


@pytest.mark.usefixtures("db_session")
Expand Down

0 comments on commit 84ae82a

Please sign in to comment.