GH-36069: [Java] Ensure S3 is finalized on shutdown #36934

danepitkin · 2023-07-28T20:52:01Z

Rationale for this change

Java datasets can implicitly create an S3 filesystem, which will initialize S3 APIs. There is currently no explicit call to shutdown S3 APIs in Java, which results in a warning message being printed at runtime:

arrow::fs::FinalizeS3 was not called even though S3 was initialized. This could lead to a segmentation fault at exit

What changes are included in this PR?

Add a Java runtime shutdown hook that calls EnsureS3Finalized() via JNI. This is a noop if S3 is uninitialized or already finalized.

Are these changes tested?

Yes, reproduced with:

import org.apache.arrow.dataset.file.FileFormat;
import org.apache.arrow.dataset.file.FileSystemDatasetFactory;
import org.apache.arrow.dataset.jni.NativeMemoryPool;
import org.apache.arrow.dataset.source.DatasetFactory;
import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.RootAllocator;

public class DatasetModule {
    public static void main(String[] args) {
        String uri = "s3://voltrondata-labs-datasets/nyc-taxi-tiny/year=2022/month=2/part-0.parquet";
        try (
            BufferAllocator allocator = new RootAllocator();
            DatasetFactory datasetFactory = new FileSystemDatasetFactory(allocator, NativeMemoryPool.getDefault(), FileFormat.PARQUET, uri);
        ) {
            // S3 is initialized
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

I didn't think a unit test was worth adding. Let me know if you think otherwise. Reasoning:

We can't test the actual shutdown since thats a JVM thing.
We could test to see if the hook is registered, but that involves exposing the API and having access to the thread object registered with the hook. Or using reflection to obtain it. Not worth it IMO.
No need to test the functionality inside the hook, its just a wrapper around a single C++ API with no params/retval.

Are there any user-facing changes?

No

Closes: [Java] while using s3 FileSystemDatasetFactory getting this exception #36069

github-actions · 2023-07-28T20:52:26Z

⚠️ GitHub issue #36069 has been automatically assigned in GitHub to PR creator.

danepitkin · 2023-07-28T20:54:35Z

java/dataset/src/main/java/org/apache/arrow/dataset/file/FileSystemDatasetFactory.java

 */
 public class FileSystemDatasetFactory extends NativeDatasetFactory {

+  private static final AtomicBoolean addedS3ShutdownHook = new AtomicBoolean(false);


I added an atomic check to ensure we only register a single hook. Without it, we could register multiple hooks to call EnsureS3Finalized.

Can we just stick it in JniLoader? https://github.com/apache/arrow/blob/main/java/dataset/src/main/java/org/apache/arrow/dataset/jni/JniLoader.java

Ah yes, this is a much better spot for this. Will update. The atomic bool did feel like overkill

danepitkin · 2023-07-28T20:56:37Z

@github-actions crossbow submit java

github-actions · 2023-07-28T20:59:24Z

Revision: 6058031

Submitted crossbow builds: ursacomputing/crossbow @ actions-389a7e4e18

Task	Status
java-jars
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-java-linux-conda-latest-amd64
verify-rc-source-java-linux-ubuntu-20.04-amd64
verify-rc-source-java-linux-ubuntu-22.04-amd64
verify-rc-source-java-macos-amd64

danepitkin · 2023-07-31T18:51:46Z

@github-actions crossbow submit java

github-actions · 2023-07-31T18:54:40Z

Revision: 556a822

Submitted crossbow builds: ursacomputing/crossbow @ actions-ebfb41f456

Task	Status
java-jars
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-java-linux-conda-latest-amd64
verify-rc-source-java-linux-ubuntu-20.04-amd64
verify-rc-source-java-linux-ubuntu-22.04-amd64
verify-rc-source-java-macos-amd64

febinsathar · 2023-08-07T15:08:49Z

thanks

conbench-apache-arrow · 2023-08-10T12:45:04Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 3501964.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change Java datasets can implicitly create an S3 filesystem, which will initialize S3 APIs. There is currently no explicit call to shutdown S3 APIs in Java, which results in a warning message being printed at runtime: `arrow::fs::FinalizeS3 was not called even though S3 was initialized. This could lead to a segmentation fault at exit` ### What changes are included in this PR? * Add a Java runtime shutdown hook that calls `EnsureS3Finalized()` via JNI. This is a noop if S3 is uninitialized or already finalized. ### Are these changes tested? Yes, reproduced with: ``` import org.apache.arrow.dataset.file.FileFormat; import org.apache.arrow.dataset.file.FileSystemDatasetFactory; import org.apache.arrow.dataset.jni.NativeMemoryPool; import org.apache.arrow.dataset.source.DatasetFactory; import org.apache.arrow.memory.BufferAllocator; import org.apache.arrow.memory.RootAllocator; public class DatasetModule { public static void main(String[] args) { String uri = "s3://voltrondata-labs-datasets/nyc-taxi-tiny/year=2022/month=2/part-0.parquet"; try ( BufferAllocator allocator = new RootAllocator(); DatasetFactory datasetFactory = new FileSystemDatasetFactory(allocator, NativeMemoryPool.getDefault(), FileFormat.PARQUET, uri); ) { // S3 is initialized } catch (Exception e) { e.printStackTrace(); } } } ``` I didn't think a unit test was worth adding. Let me know if you think otherwise. Reasoning: * We can't test the actual shutdown since thats a JVM thing. * We could test to see if the hook is registered, but that involves exposing the API and having access to the thread object registered with the hook. Or using reflection to obtain it. Not worth it IMO. * No need to test the functionality inside the hook, its just a wrapper around a single C++ API with no params/retval. ### Are there any user-facing changes? No * Closes: apache#36069 Authored-by: Dane Pitkin <dane@voltrondata.com> Signed-off-by: David Li <li.davidm96@gmail.com>

apacheGH-36069: [Java] Ensure S3 is finalized on shutdown

6058031

danepitkin requested a review from lidavidm as a code owner July 28, 2023 20:52

github-actions bot added Component: Java awaiting review Awaiting review labels Jul 28, 2023

danepitkin commented Jul 28, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 28, 2023

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 28, 2023

Move implementation to JniLoader

556a822

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 31, 2023

lidavidm approved these changes Jul 31, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 31, 2023

lidavidm merged commit 3501964 into apache:main Aug 2, 2023

lidavidm removed the awaiting merge Awaiting merge label Aug 2, 2023

t-kalinowski mentioned this pull request Oct 4, 2023

Segfault with r-arrow=12 rstudio/reticulate#1470

Closed

kou mentioned this pull request May 27, 2024

GH-41836: [Java] Fix an undefined symbol error when ARROW_S3=OFF #41837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36069: [Java] Ensure S3 is finalized on shutdown #36934

GH-36069: [Java] Ensure S3 is finalized on shutdown #36934

Uh oh!

danepitkin commented Jul 28, 2023 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Jul 28, 2023

Uh oh!

danepitkin Jul 28, 2023

Uh oh!

lidavidm Jul 28, 2023

Uh oh!

danepitkin Jul 31, 2023

Uh oh!

danepitkin commented Jul 28, 2023

Uh oh!

github-actions bot commented Jul 28, 2023

Uh oh!

danepitkin commented Jul 31, 2023

Uh oh!

github-actions bot commented Jul 31, 2023

Uh oh!

febinsathar commented Aug 7, 2023

Uh oh!

conbench-apache-arrow bot commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GH-36069: [Java] Ensure S3 is finalized on shutdown #36934

GH-36069: [Java] Ensure S3 is finalized on shutdown #36934

Uh oh!

Conversation

danepitkin commented Jul 28, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jul 28, 2023

Uh oh!

danepitkin Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

lidavidm Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

danepitkin Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

danepitkin commented Jul 28, 2023

Uh oh!

github-actions bot commented Jul 28, 2023

Uh oh!

danepitkin commented Jul 31, 2023

Uh oh!

github-actions bot commented Jul 31, 2023

Uh oh!

febinsathar commented Aug 7, 2023

Uh oh!

conbench-apache-arrow bot commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danepitkin commented Jul 28, 2023 •

edited by github-actions bot

Loading