From 944238e9c0236edeed6187c87c4a72c10f573500 Mon Sep 17 00:00:00 2001 From: Logan Gore Date: Thu, 1 Sep 2022 15:41:14 -0700 Subject: [PATCH] Use singleton per-region for S3Client Summary: # What * Use a singleton for each region when constructing our S3Client instead of a _single_ singleton * This is kind of a follow-up to D36727729 (https://github.com/facebookresearch/fbpcf/commit/47182c669b94f972d401d2acab937e971219034c) # Why * Follow-up to S281873 - S3Client singleton breaks multi-region support in PCF IO NOTE: PCF should be owning this code in the long-term since it's out of PSI's scope, but since we owned the initial SEV, I took ownership of this follow-up. Differential Revision: D39174441 fbshipit-source-id: 28c728b573669cccb01d91139529f289d6777f38 --- fbpcf/io/cloud_util/S3Client.cpp | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/fbpcf/io/cloud_util/S3Client.cpp b/fbpcf/io/cloud_util/S3Client.cpp index 83fddb9c..a30b5ebb 100644 --- a/fbpcf/io/cloud_util/S3Client.cpp +++ b/fbpcf/io/cloud_util/S3Client.cpp @@ -7,12 +7,37 @@ #include "fbpcf/io/cloud_util/S3Client.h" +#include + #include #include +#include + namespace fbpcf::cloudio { S3Client& S3Client::getInstance(const fbpcf::aws::S3ClientOption& option) { - static S3Client s3Client(option); - return s3Client; + /* Due to previous problems, we create a Singleton instance of the S3Client, + * but there's a catch: we need a distinct S3Client for each region, or we + * run into other issues. For that reason, we store this map from string to + * S3Client with the assumption that the keys are region names. Since region + * is optional, we also allow for a default empty string region. + * ***************************** NOT THREAD SAFE **************************** + * NOTE: Significant refactoring is required to make this thread safe + * Downstream usage wants a mutable reference, but a folly::Synchronized + * RWLock will return a const ref to a reader, meaning it's hard to refactor. + * Simply trying to use folly::Synchronized around the map isn't sufficient, + * because we'll leak a reference to an object in the map which is unsafe. + * ***************************** NOT THREAD SAFE **************************** + */ + static folly::F14FastMap m; + + std::string defaultStr{}; + auto region = option.region.value_or(defaultStr); + + if (m.find(region) == m.end()) { + m.at(region) = S3Client{option}; + } + + return m.at(region); } } // namespace fbpcf::cloudio