Skip to content

Commit

Permalink
Use singleton per-region for S3Client
Browse files Browse the repository at this point in the history
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 (47182c6)
# 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
  • Loading branch information
Logan Gore authored and facebook-github-bot committed Sep 1, 2022
1 parent 6fa554d commit 944238e
Showing 1 changed file with 27 additions and 2 deletions.
29 changes: 27 additions & 2 deletions fbpcf/io/cloud_util/S3Client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,37 @@

#include "fbpcf/io/cloud_util/S3Client.h"

#include <string>

#include <aws/core/Aws.h>
#include <aws/s3/S3Client.h>

#include <folly/container/F14Map.h>

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<std::string, S3Client> 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

0 comments on commit 944238e

Please sign in to comment.