You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/07/03 05:15:18 UTC
[GitHub] [arrow] kou opened a new pull request, #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
kou opened a new pull request, #36437:
URL: https://github.com/apache/arrow/pull/36437
### Rationale for this change
`arrow::FinalizeS3()` doesn't call both of `RegionResolver::ResetDefaultInstance()` and `Aws::ShutdownAPI()` by #33858.
If we don't call both of them, some aws-sdk-cpp related objects are destroyed on exit. It may cause a crash.
### What changes are included in this PR?
This calls both of them by `arrow::FinalizeS3()` again to prevent crash on exit. All S3 related operations are failed after we call `arrow::fs::FinalizeS3()`.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #36437:
URL: https://github.com/apache/arrow/pull/36437#discussion_r1251283717
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -398,12 +398,19 @@ namespace {
Status CheckS3Initialized() {
if (!IsS3Initialized()) {
return Status::Invalid(
- "S3 subsystem not initialized; please call InitializeS3() "
+ "S3 subsystem is not initialized; please call InitializeS3() "
"before carrying out any S3-related operation");
}
return Status::OK();
}
+Status CheckS3Finalized() {
Review Comment:
Ah, you're right.
We don't need this and I should have added `Not`...
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -2562,6 +2626,8 @@ Result<std::shared_ptr<io::OutputStream>> S3FileSystem::OpenOutputStream(
ARROW_ASSIGN_OR_RAISE(auto path, S3Path::FromString(s));
RETURN_NOT_OK(ValidateFilePath(path));
+ RETURN_NOT_OK(CheckS3Finalized());
Review Comment:
Yes.
(I chose this position to use non aws-sdk-cpp related codes as much as possible even after `arrow::fs::FinalizeS3()`. But it may not be important in most cases.)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36437:
URL: https://github.com/apache/arrow/pull/36437#issuecomment-1617359529
:warning: GitHub issue #36346 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36437:
URL: https://github.com/apache/arrow/pull/36437#discussion_r1250758134
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -398,12 +398,19 @@ namespace {
Status CheckS3Initialized() {
if (!IsS3Initialized()) {
return Status::Invalid(
- "S3 subsystem not initialized; please call InitializeS3() "
+ "S3 subsystem is not initialized; please call InitializeS3() "
"before carrying out any S3-related operation");
}
return Status::OK();
}
+Status CheckS3Finalized() {
Review Comment:
In any case, please call this `CheckS3NotFinalized` as the current name is misleading.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -398,12 +398,19 @@ namespace {
Status CheckS3Initialized() {
if (!IsS3Initialized()) {
return Status::Invalid(
- "S3 subsystem not initialized; please call InitializeS3() "
+ "S3 subsystem is not initialized; please call InitializeS3() "
"before carrying out any S3-related operation");
}
return Status::OK();
}
+Status CheckS3Finalized() {
Review Comment:
Is this actually useful? `is_initialized_` is set to false when finalizing, so `CheckS3Initialized` will already fail.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -2562,6 +2626,8 @@ Result<std::shared_ptr<io::OutputStream>> S3FileSystem::OpenOutputStream(
ARROW_ASSIGN_OR_RAISE(auto path, S3Path::FromString(s));
RETURN_NOT_OK(ValidateFilePath(path));
+ RETURN_NOT_OK(CheckS3Finalized());
Review Comment:
Can you put this consistently at the start of the method?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36437:
URL: https://github.com/apache/arrow/pull/36437#issuecomment-1618905582
Alternative PR: #36442
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36437:
URL: https://github.com/apache/arrow/pull/36437#issuecomment-1618396510
It appears this fix doesn't work properly, as the test I added crashes:
https://gist.github.com/pitrou/bde45f7a213bf4016cff454ad69a3653
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36437:
URL: https://github.com/apache/arrow/pull/36437#issuecomment-1618740378
I'm working on a slightly different approach.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou closed pull request #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou closed pull request #36437: GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize
URL: https://github.com/apache/arrow/pull/36437
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org