Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s3x - issue with loading files content from localstack #236

Closed
thsandu opened this issue Oct 27, 2023 · 7 comments
Closed

s3x - issue with loading files content from localstack #236

thsandu opened this issue Oct 27, 2023 · 7 comments

Comments

@thsandu
Copy link

thsandu commented Oct 27, 2023

We want to use the newly implemented s3x functionality to have integration tests with localstack. A test will fire up the localstack container, copy some files to the container and then have our software load the data from localstack via the provided URI. The copy and Files.exists works well, but I have troubles accessing the file content from localstack.

I forked your repo to reproduce the issue with a new integration test: https://github.com/thsandu/aws-java-nio-spi-for-s3-issues-with-s3x/blob/main/src/integrationTest/java/software/amazon/nio/spi/s3/FilesCopyTest.java

The Files.copy in the following code produces an exception

final Path path = Paths.get(URI.create(localStackConnectionEndpoint() + "/sink/sample-file.txt"));
Path copiedFile = Files.copy(path, tempDir.resolve("sample-file-local.txt"));
then(Files.exists(path)).isTrue();
then(Files.exists(copiedFile));
java.io.IOException: java.util.concurrent.ExecutionException: software.amazon.awssdk.core.exception.SdkClientException: Failed to send the request: Response missing required Content-Range header.
	at software.amazon.nio.spi.s3.S3ReadAheadByteChannel.read(S3ReadAheadByteChannel.java:155)
	at software.amazon.nio.spi.s3.S3SeekableByteChannel.read(S3SeekableByteChannel.java:135)
	at java.base/sun.nio.ch.ChannelInputStream.read(ChannelInputStream.java:65)
	at java.base/sun.nio.ch.ChannelInputStream.read(ChannelInputStream.java:107)
	at java.base/sun.nio.ch.ChannelInputStream.read(ChannelInputStream.java:101)
	at java.base/java.io.InputStream.transferTo(InputStream.java:704)
	at java.base/java.nio.file.Files.copy(Files.java:3078)
	at java.base/java.nio.file.CopyMoveHelper.copyToForeignTarget(CopyMoveHelper.java:126)
	at java.base/java.nio.file.Files.copy(Files.java:1298)
	at software.amazon.nio.spi.s3.FilesCopyTest$FileExists.fileCopyShouldCopyFileWhenFileFound(FilesCopyTest.java:56)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:728)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$7(TestMethodTestDescriptor.java:218)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:214)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:139)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:69)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:151)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:108)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:88)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:67)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:52)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:96)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:75)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy5.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
Caused by: java.util.concurrent.ExecutionException: software.amazon.awssdk.core.exception.SdkClientException: Failed to send the request: Response missing required Content-Range header.
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:395)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2022)
	at software.amazon.nio.spi.s3.S3ReadAheadByteChannel.read(S3ReadAheadByteChannel.java:110)
	... 102 more
Caused by: software.amazon.awssdk.core.exception.SdkClientException: Failed to send the request: Response missing required Content-Range header.
	at app//software.amazon.awssdk.core.exception.SdkClientException$BuilderImpl.build(SdkClientException.java:111)
	at app//software.amazon.awssdk.core.exception.SdkClientException.create(SdkClientException.java:43)
	at app//software.amazon.awssdk.services.s3.internal.crt.S3CrtResponseHandlerAdapter.handleError(S3CrtResponseHandlerAdapter.java:135)
	at app//software.amazon.awssdk.services.s3.internal.crt.S3CrtResponseHandlerAdapter.onFinished(S3CrtResponseHandlerAdapter.java:101)
	at app//software.amazon.awssdk.crt.s3.S3MetaRequestResponseHandlerNativeAdapter.onFinished(S3MetaRequestResponseHandlerNativeAdapter.java:24)

Same result I get also for the other ways to read the file content on the localstack, as you could see in the other two tests in the class.

guicamest added a commit to guicamest/aws-java-nio-spi-for-s3 that referenced this issue Nov 2, 2023
guicamest added a commit to guicamest/aws-java-nio-spi-for-s3 that referenced this issue Nov 2, 2023
@guicamest
Copy link
Contributor

I've created an up-to-date PR of the tests with a couple of fixes in the assertions (comparing bytes with string for example).

Discovery

I've tried with different versions of localstack (2.0.x up until 2.3.x), all resulted in the same error :(

After some research, I came across this issue in the localstack repository, pointing towards a problem when using crt + range.

When running fileReadAllBytesShouldReturnFileContentsWhenFileFound, these are the requests received by localstack.

Two quick changes make the tests pass:

  1. Remove the .range() call in S3ReadAheadByteChannel
                        builder -> builder
                                .bucket(path.bucketName())
+                               .key(path.getKey()),
-                               .key(path.getKey())
-                               .range(range),

This means that when using the crt client to get the object from S3, the request works. But it removes the range, thus the purpose of the ReadAhead shatters.

When running fileReadAllBytesShouldReturnFileContentsWhenFileFound, these are the requests received by localstack.

  1. Initialize a non-crt async client in clientForRegion
+ S3AsyncClientBuilder asyncClientBuilder = S3AsyncClient.builder();
  if (!endpoint.isBlank()) {
      asyncClientBuilder.endpointOverride(URI.create(configuration.getEndpointProtocol() + "://" + endpoint));
  }

This change causes the library to use the default (non-crt) for all requests. Also, there are 2 unit tests that are broken due to the change: generateAsyncClientByEndpointBucketCredentials and setCredentialsThroughURI.

When running fileReadAllBytesShouldReturnFileContentsWhenFileFound, these are the requests received by localstack.

Cause

Looking at the different request logs, it seems that when using crt and range is used for getObject, the client "incorrectly" adds the Range header to the HEAD request before the GET request, causing localstack to complain.

To find out

  • Is it only localstack that is complaining? In other words, using the latest version of the library to read a file from an actual AWS S3 bucket using Files.readAllBytes works? @markjschreiber could you test this (don't have access to a live bucket unfortunately :( )
  • Is this a bug in crt (it is version 0.2x afterall)?

Discussion

Removing the range from the request (Option 1) breaks the whole purpose of the ReadAhead class, so I wouldn't like to consider doing that in order to fix this issue.

Using the default (non-crt) client (Option 2) seems a better alternative. S3ReadAheadByteChannel is for reading, not necessarily the whole Object, so there wouldn't be any improved throughput by using the crt client in this particular use case.

@markjschreiber what are your thoughts?

@markjschreiber
Copy link
Contributor

I'll test with a vanilla s3 bucket to see if the issue is specific to localstack.

If needed we could drop our use of crt and use the default client.

@markjschreiber
Copy link
Contributor

Confirmed that this works

package software.amazon.nio.spi.examples;

import java.io.IOException;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;

public class ReadAllBytes {
    public static void main(String[] args) throws IOException {
        Path filePath = Paths.get(URI.create(args[0]));

        final byte[] bytes = Files.readAllBytes(filePath);
        // assumes this is a text file
        final String data = new String(bytes, StandardCharsets.UTF_8);
        System.out.println(data);
    }
}

@guicamest
Copy link
Contributor

Thanks for checking it @markjschreiber ! I'll make the changes so that S3ReadAheadByteChannel uses the non-crt client. S3WritableByteChannel still needs the crt client to download the temporary file using transfermanager.

guicamest added a commit to guicamest/aws-java-nio-spi-for-s3 that referenced this issue Nov 3, 2023
guicamest added a commit to guicamest/aws-java-nio-spi-for-s3 that referenced this issue Nov 3, 2023
@guicamest
Copy link
Contributor

@thsandu The issue should be fixed by #252 , can you check it out?

@thsandu
Copy link
Author

thsandu commented Nov 6, 2023

@thsandu The issue should be fixed by #252 , can you check it out?

I did a small test and the File.copy worked with the lib based on your fork. Thank you!

markjschreiber pushed a commit that referenced this issue Nov 6, 2023
* test(Files): Add integration tests for Files.copy / read* from issue #236 [skip ci]

* chore(S3ClientProvider): reformat

* rewrite: Extract `getRegionFromRegionName` and `endpointURI` methods

* chore: Use `isBlank` instead of `trim().isEmpty()`

* rewrite(S3ClientProvider): Extract method to configure and build client using S3BaseClientBuilder

* fix(236): Use non-crt async client for `S3ReadAheadByteChannel`

* chore: extract configureCrtClientForRegion method
@markjschreiber
Copy link
Contributor

Closing as #252 is now merged to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants