AWS: abort S3 input stream on close if not EOS #7262

bryanck · 2023-04-01T23:56:51Z

This PR adds a check when closing an S3 input stream if the stream is at end-of-stream. If not, it calls abort() on the stream instead of close(). This avoids reading to the end of the stream when closing.

The Apache HTTP connection will always read the entire stream on close, which results in much more data being read than needed. The URL connection client does not behave this way.

Now that the Apache HTTP client is the default instead of URL connection client for AWS, this change should prevent a performance regression.

danielcweeks · 2023-04-02T16:45:19Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

+      try {
+        stream.close();
+      } catch (Exception e) {
+        // close quietly


We might want to log the underlying reason as info or warn here, just so we don't silently swallow issues that may come up.

This will always throw on an aborted stream on a checksum check failure, so it could be very noisy

I added a trace log

danielcweeks · 2023-04-02T16:48:05Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputStream.java

-    if (stream != null) {
-      stream.close();
-    }
+    closeStream();


This change seems unrelated and I'm not sure what it really adds. It looks like we're dereferencing the stream, but we still would throw in any situation where it actually have any affect.

stream.close() was being called without the null check in one place, so this was added to prevent a potential NPE, however this is unrelated to the performance regression.

I rolled this back

singhpk234 · 2023-04-02T17:34:53Z

Thanks @bryanck, this is great find and fix !

one doubt will it cause issue when we switch back the client from apache to url-connection (let's say via table props) considering this was not a issue with url-connection client ?

Also do we need this in 1.2.1 considering 1.2.0 was still shipped with url-connection as default http client ? The regression commit is still in master commit.

bryanck · 2023-04-02T17:44:47Z

@singhpk234 The abort() is a no-op with the URL connection client, so it should behave mostly the same as before when using that client (except with the added read for the EOS check).

I thought we wanted to get #7119 into 1.2.1, is that still the case @danielcweeks @nastra ? Without that, it becomes more cumbersome to configure when using the AWS bundle, as including the URL connection client on the classpath then requires the default AWS http client be set also (e.g. via system properties).

singhpk234 · 2023-04-02T18:21:43Z

@bryanck apologies, as per my understanding, I thought it's other way round, based on this file UrlConnectionHttpClient.java abort() actually disconnects the connection and in the close it's a no-op close()

I am also very curious about this statement from the doc as well :

"Input stream that provides access to the unmarshalled POJO response returned by the service in addition to the streamed contents. This input stream should be closed to release the underlying connection back to the connection pool.

If it is not desired to read remaining data from the stream, you can explicitly abort the connection via abort(). Note that this will close the underlying connection and require establishing an HTTP connection which may outweigh the cost of reading the additional data."

If we go by the last paragraph and IIUC, it says calling abort will not let the connection being re-used and the connection pool will have to establish a new http connection, can it cause regression in some scenario ?

I am just trying to clear my understanding here.

I thought we wanted to get #7119 into 1.2.1, is that still the case

My understanding was 1.2.1 was only for bug fixes for 1.2.0 release.

bryanck · 2023-04-02T19:51:11Z

@singhpk234 abort() is being called on the stream, in this case it is a ResponseInputStream, rather than on the client. You are correct that calling abort() will render the connection not reusable, so there is a trade off. I tried to balance that by checking for EOS and only aborting if we aren't at the EOS, so in cases we are we can reuse the connection. The trade off is reading a bunch of data unnecessarily vs reusing the connection.

bryanck · 2023-04-02T19:54:20Z

I ran the TPC-DS benchmark with and without the changes in this PR using the Apache HTTP client. Without this change, the result was ~2x slower (as a result of reading far more data). With this change, the result was the same as with URL connection client (and the amount of data transferred was also the same). Closing the stream opened by URL connection client won't read to EOS like with the Apache HTTP client, so with this PR the behavior should be better aligned.

bryanck · 2023-04-03T16:27:42Z

@singhpk234 you are right that changing the default HTTP client for a patch release is not appropriate, so we'd leave that out of 1.2.1. We'd like the Apache HTTP client fixes in for 1.2.1, though, so that can be used.

jackye1995 · 2023-04-03T19:03:25Z

Nice catch.

Sorry I totally forgot we did exactly the same abort thing for Trino: https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/TrinoS3FileSystem.java#L1579,

and also in my old PR https://github.com/apache/iceberg/pull/4912/files#diff-0b632866a3b10fac55c442b08178ec0ac72b3b600878243e15d788a8bd031054R299.

jackye1995 · 2023-04-03T19:06:30Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

+
+  private void abortStream() {
+    try {
+      if (stream instanceof Abortable && stream.read() != -1) {


why do we want to read one more byte here? It might cause one more request. I think it does not hurt to even abort when it's already fully read.

If you abort the connection then it will be invalidated and removed from the pool, so it won't be reused. So this is an optimization to attempt to reuse the connection in cases where it has been fully read.

jackye1995 · 2023-04-03T19:13:38Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

+      } catch (Exception e) {
+        // log at trace level as closing an aborted stream will throw a content length
+        // check exception with the Apache HTTP client
+        LOG.trace("Error closing stream", e);


That means we will see exceptions for every stream close if trace is enabled. If it is expected to fail with the content length check, can we at least skip that? Then we don't need to log in trace, but we can log in maybe a warning level.

That is how I originally had it but the feedback was to log something. I'd be happy to remove that.

@singhpk234 @danielcweeks are you ok if I revert this back to a no-op?

I guess the question to ask before this is, why do we need to close it again if it's already aborted? If you see in the Trino logic I referenced, it's an if-else.

That would work for the Apache HTTP client, the close doesn't do anything for that. But aborting the stream opened by URL connection client doesn't do anything, so we need to close it in that case.

I see, that's interesting difference, thanks for the explanation!

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

jackye1995 · 2023-04-03T21:58:41Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

+      try {
+        stream.close();
+      } catch (Exception e) {
+        // close quietly, closing an aborted stream will throw a content length


Sorry I guess I did not express it clearly. What I meant was that, can we check for the exception that if it is a content length check exception then we can skip, otherwise we log a warning?

I see. ContentLengthInputStream is an Apache HTTP class, so referring to that will tie S3InputStream to that client.

I could check the name of the class, but that seems like a little bit of a hack.

Ah okay, what does the exception look like? I though there are some error code that we can get out of the exception that would imply this specific error. Is that not there?

This is what gets thrown. Calling close on the aborted stream will attempt a read which then throws that.

Checking ConnectionClosedException seems good enough to me, because it makes sense that if the connection is already closed then we can omit the error.

I think org.apache.http.ConnectionClosedException is already in the class path given we have some REST client integration in core, but I might be wrong.

Sounds good, I pushed that change

That is a different version than being used by the AWS SDK (v5 instead of v4). Also, the AWS bundle shades the library.

Another option is we check which HTTP client is being used and change the close behavior based on that.

jackye1995

Thanks, looks good to me!

amogh-jahagirdar · 2023-04-03T22:48:17Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

+      } catch (IOException e) {
+        // the Apache HTTP client will throw a ConnectionClosedException
+        // when closing an aborted stream, which is expected
+        if (!e.getClass().getSimpleName().equals("ConnectionClosedException")) {


Can we just do instanceof here?

Oh okay I see this thread #7262 (comment), since it's specific to Apache Http client, it's not guaranteed it'll be on the classpath so we can't use instanceof

That would add a dependency on the AWS-specific Apache HTTP client (v4). Also, the AWS bundle shades the library, so the classes will be different depending on that (different packages).

amogh-jahagirdar

LGTM, thanks for the fix @bryanck !

jackye1995 · 2023-04-04T00:37:46Z

Another option is we check which HTTP client is being used and change the close behavior based on that.

Actually that's a potentially better approach, since we only have 2 officially supported client types apache and urlconnection, and their behaviors are quite different based on this exploration.

I am a bit worried about the 1 additional byte read, seems to only benefit urlconnection but could be safe or worse for apache. Switch behavior by client type would solve the issue.

We could get the value from awsProperties, but one issue is that for users with a custom client factory, they might overwrote the entire client factory and thus have a HTTP client type mismatch. But even in that case, user can explicitly set the HTTP client to change the behavior, so it seems to be a safe approach.

@bryanck @amogh-jahagirdar any thoughts?

bryanck · 2023-04-04T00:39:31Z

I was hoping we could get it from S3Client but I didn't see a way, I'll look some more tonight

bryanck · 2023-04-04T08:50:51Z

The only way I could find to determine the HTTP client type on the S3 client is to use reflection. So we could do that which is not ideal. Another options is to read the AwsProperties to get the type, but that may not match the actual HTTP client for custom factories. I feel like the current solution is the least invasive for the 1.2.1 release and achieves the desired outcome of performance parity between the two HTTP clients.

There are alternatives to detecting the EOS also. We could get the content length of the response and track the number of bytes read from the stream, but I feel like that is heavier than the one byte read. Generaly that byte will be buffered if not EOS so shouldn't trigger any network I/O.

* AWS: abort S3 input stream on close if not EOS * Close the stream for backwards compatibility * undo unrelated change * add trace log * comment update * logger updates * handle connection closed exception

singhpk234

LGTM as well (apologies being late to party), many thanks @bryanck for this fix !

* AWS: abort S3 input stream on close if not EOS * Close the stream for backwards compatibility * undo unrelated change * add trace log * comment update * logger updates * handle connection closed exception

AWS: abort S3 input stream on close if not EOS

3939b55

github-actions bot added the AWS label Apr 1, 2023

danielcweeks added this to the Iceberg 1.2.1 milestone Apr 2, 2023

Close the stream for backwards compatibility

cf247eb

danielcweeks reviewed Apr 2, 2023

View reviewed changes

bryanck added 2 commits April 2, 2023 10:30

undo unrelated change

492c419

add trace log

ac2e5df

comment update

d365d91

nastra approved these changes Apr 3, 2023

View reviewed changes

jackye1995 reviewed Apr 3, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java Outdated Show resolved Hide resolved

logger updates

69d8f1a

jackye1995 reviewed Apr 3, 2023

View reviewed changes

handle connection closed exception

f881681

jackye1995 approved these changes Apr 3, 2023

View reviewed changes

amogh-jahagirdar reviewed Apr 3, 2023

View reviewed changes

amogh-jahagirdar approved these changes Apr 3, 2023

View reviewed changes

danielcweeks approved these changes Apr 4, 2023

View reviewed changes

danielcweeks merged commit 49e9308 into apache:master Apr 4, 2023

singhpk234 reviewed Apr 4, 2023

View reviewed changes

singhpk234 mentioned this pull request Jun 23, 2025

feat: Support customizing S3 endpoints apache/polaris#1913

Merged

nastra mentioned this pull request Oct 15, 2025

Uses content length to determine when to abort the stream. #14329

Closed

AWS: abort S3 input stream on close if not EOS #7262

AWS: abort S3 input stream on close if not EOS #7262

Uh oh!

Conversation

bryanck commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 commented Apr 2, 2023

Uh oh!

bryanck commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singhpk234 commented Apr 2, 2023

Uh oh!

bryanck commented Apr 2, 2023

Uh oh!

bryanck commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanck commented Apr 3, 2023

Uh oh!

jackye1995 commented Apr 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanck Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

bryanck commented Apr 1, 2023 •

edited

Loading

bryanck commented Apr 2, 2023 •

edited

Loading

bryanck commented Apr 2, 2023 •

edited

Loading

bryanck Apr 3, 2023 •

edited

Loading