kou commented on code in PR #36442:
URL: https://github.com/apache/arrow/pull/36442#discussion_r1251414361
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -789,6 +795,136 @@ class ClientBuilder {
std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credentials_provider_;
};
+// -----------------------------------------------------------------------
+// S3 client protection against use after finalization
+//
+// It is extremely recommended to call FinalizeS3() before process end.
+// However, once this is done, AWS APIs cannot reliably be called anymore
+// (even destructors may crash or behave randomly). To avoid such occurrences,
+// we wrap all S3Client instances in a special structure (S3ClientHolder)
+// that prevents usage of S3Client after FinalizeS3() did its job.
+
+class S3ClientFinalizer;
+
+class S3ClientLock {
+ public:
+ S3Client* operator*() { return client_.get(); }
Review Comment:
How about using `get()` instead of `operator*()`? Because other
`operator*()` such as `std::shared_ptr::operator*()` uses `T&` not `T*`. The
difference may confuse us.
```suggestion
S3Client* get() { return client_.get(); }
```
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -789,6 +795,136 @@ class ClientBuilder {
std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credentials_provider_;
};
+// -----------------------------------------------------------------------
+// S3 client protection against use after finalization
+//
+// It is extremely recommended to call FinalizeS3() before process end.
+// However, once this is done, AWS APIs cannot reliably be called anymore
+// (even destructors may crash or behave randomly). To avoid such occurrences,
+// we wrap all S3Client instances in a special structure (S3ClientHolder)
+// that prevents usage of S3Client after FinalizeS3() did its job.
+
+class S3ClientFinalizer;
+
+class S3ClientLock {
+ public:
+ S3Client* operator*() { return client_.get(); }
+ S3Client* operator->() { return client_.get(); }
+
+ S3ClientLock() = default;
+ ARROW_DISALLOW_COPY_AND_ASSIGN(S3ClientLock);
+ ARROW_DEFAULT_MOVE_AND_ASSIGN(S3ClientLock);
+
+ protected:
+ friend class S3ClientHolder;
+
+ // Locks the finalizer until the S3ClientLock gets out of scope.
+ std::shared_lock<std::shared_mutex> lock_;
+ std::shared_ptr<S3Client> client_;
+};
+
+class S3ClientHolder {
+ public:
+ /// \brief Return a RAII guard guaranteeing a S3Client is safe for use
+ ///
+ /// S3 finalization will be deferred until the returned S3ClientLock
+ /// goes out of scope.
+ /// An error is returned if S3 is already finalized.
+ Result<S3ClientLock> Lock();
+
+ protected:
+ void Finalize();
+
+ friend class S3ClientFinalizer;
+
+ std::mutex mutex_;
+ std::weak_ptr<S3ClientFinalizer> finalizer_;
+ std::shared_ptr<S3Client> client_;
+};
+
+class S3ClientFinalizer : public
std::enable_shared_from_this<S3ClientFinalizer> {
+ using ClientHolderList = std::vector<std::weak_ptr<S3ClientHolder>>;
+
+ public:
+ Result<std::shared_ptr<S3ClientHolder>> AddClient(std::shared_ptr<S3Client>
client) {
+ std::unique_lock lock(mutex_);
+ if (finalized_) {
+ return ErrorS3Finalized();
+ }
+
+ auto ptr = std::make_shared<S3ClientHolder>();
Review Comment:
`holder` instead of `ptr` may be better because we use `holder`/`holder_` in
other places.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -2581,17 +2773,18 @@ Result<std::shared_ptr<io::OutputStream>>
S3FileSystem::OpenAppendStream(
namespace {
-struct AwsInstance : public ::arrow::internal::Executor::Resource {
+struct AwsInstance {
AwsInstance() : is_initialized_(false), is_finalized_(false) {}
~AwsInstance() { Finalize(/*from_destructor=*/true); }
// Returns true iff the instance was newly initialized with `options`
Result<bool> EnsureInitialized(const S3GlobalOptions& options) {
- bool expected = false;
+ // XXX The individual accesses are atomic but the entire sequence below is
not.
Review Comment:
I agree with your opinion.
How about adding this note to this comment or documents for
`InitializeS3()`/`FinalizeS3()`?
(I think that we can remove the "XXX" mark here.)
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -2670,21 +2868,13 @@ struct AwsInstance : public
::arrow::internal::Executor::Resource {
std::atomic<bool> is_finalized_;
};
-std::shared_ptr<AwsInstance> CreateAwsInstance() {
- auto instance = std::make_shared<AwsInstance>();
- // Don't let S3 be shutdown until all Arrow threads are done using it
- arrow::internal::GetCpuThreadPool()->KeepAlive(instance);
- io::internal::GetIOThreadPool()->KeepAlive(instance);
- return instance;
-}
-
-AwsInstance& GetAwsInstance() {
- static auto instance = CreateAwsInstance();
- return *instance;
+AwsInstance* GetAwsInstance() {
+ static auto instance = std::make_shared<AwsInstance>();
Review Comment:
We may want to use `std::make_unique()` instead because this smart pointer
isn't shared.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -849,13 +986,15 @@ class RegionResolver {
DCHECK(builder_.options().endpoint_override.empty());
// On Windows with AWS SDK >= 1.8, it is necessary to disable redirects
(ARROW-10085).
DisableRedirects(builder_.mutable_config());
- return builder_.BuildClient().Value(&client_);
+ ARROW_ASSIGN_OR_RAISE(auto client, builder_.BuildClient());
+ ARROW_ASSIGN_OR_RAISE(holder_, GetClientHolder(std::move(client)));
Review Comment:
How about doing this in `ClientBuilder::BuildClient()` instead of here?
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -789,6 +795,136 @@ class ClientBuilder {
std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credentials_provider_;
};
+// -----------------------------------------------------------------------
+// S3 client protection against use after finalization
+//
+// It is extremely recommended to call FinalizeS3() before process end.
+// However, once this is done, AWS APIs cannot reliably be called anymore
+// (even destructors may crash or behave randomly). To avoid such occurrences,
+// we wrap all S3Client instances in a special structure (S3ClientHolder)
+// that prevents usage of S3Client after FinalizeS3() did its job.
+
+class S3ClientFinalizer;
+
+class S3ClientLock {
+ public:
+ S3Client* operator*() { return client_.get(); }
+ S3Client* operator->() { return client_.get(); }
+
+ S3ClientLock() = default;
+ ARROW_DISALLOW_COPY_AND_ASSIGN(S3ClientLock);
+ ARROW_DEFAULT_MOVE_AND_ASSIGN(S3ClientLock);
+
+ protected:
+ friend class S3ClientHolder;
+
+ // Locks the finalizer until the S3ClientLock gets out of scope.
+ std::shared_lock<std::shared_mutex> lock_;
+ std::shared_ptr<S3Client> client_;
+};
+
+class S3ClientHolder {
+ public:
+ /// \brief Return a RAII guard guaranteeing a S3Client is safe for use
+ ///
+ /// S3 finalization will be deferred until the returned S3ClientLock
+ /// goes out of scope.
+ /// An error is returned if S3 is already finalized.
+ Result<S3ClientLock> Lock();
+
+ protected:
+ void Finalize();
+
+ friend class S3ClientFinalizer;
+
+ std::mutex mutex_;
+ std::weak_ptr<S3ClientFinalizer> finalizer_;
+ std::shared_ptr<S3Client> client_;
+};
+
+class S3ClientFinalizer : public
std::enable_shared_from_this<S3ClientFinalizer> {
+ using ClientHolderList = std::vector<std::weak_ptr<S3ClientHolder>>;
+
+ public:
+ Result<std::shared_ptr<S3ClientHolder>> AddClient(std::shared_ptr<S3Client>
client) {
+ std::unique_lock lock(mutex_);
+ if (finalized_) {
+ return ErrorS3Finalized();
+ }
+
+ auto ptr = std::make_shared<S3ClientHolder>();
+ ptr->finalizer_ = shared_from_this();
+ ptr->client_ = std::move(client);
Review Comment:
How about defining `S3ClientHolder::S3ClientHolder()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
