From ccebd29a7a77ca6dacc07422ac69322cecfb9613 Mon Sep 17 00:00:00 2001 From: Tomas Celaya Date: Thu, 2 Aug 2018 13:51:07 -0700 Subject: [PATCH 1/8] Make unrecoverable exception handling less confusing --- .../manta/util/AutoContinuingInputStream.java | 18 +++- .../manta/util/ContinuingInputStream.java | 92 +++++++------------ 2 files changed, 49 insertions(+), 61 deletions(-) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java index 601e4c8f..d256054d 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java @@ -8,6 +8,8 @@ package com.joyent.manta.util; import org.apache.commons.io.IOUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.io.InputStream; @@ -26,6 +28,8 @@ */ public class AutoContinuingInputStream extends ContinuingInputStream { + private static final Logger LOG = LoggerFactory.getLogger(AutoContinuingInputStream.class); + /** * Produces continuations of the original stream given new byte offsets. */ @@ -53,10 +57,12 @@ public AutoContinuingInputStream(final InputStream wrapped, * continuing */ private void attemptRecovery(final IOException originalIOException) throws IOException { + final InputStream continuation; try { - this.continueWith(this.continuator.buildContinuation(originalIOException, this.getBytesRead())); - } catch (final IOException ce) { - originalIOException.addSuppressed(ce); + continuation = this.continuator.buildContinuation(originalIOException, this.getBytesRead()); + super.continueWith(continuation); + } catch (final IOException ioe) { + LOG.debug("Failed to automatically recover: {}", ioe.getMessage()); throw originalIOException; } } @@ -135,6 +141,10 @@ public boolean markSupported() { public void close() throws IOException { IOUtils.closeQuietly(this.continuator); - this.getWrapped().close(); + final InputStream wrapped = this.getWrapped(); + + if (wrapped != null) { + wrapped.close(); + } } } diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/ContinuingInputStream.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/ContinuingInputStream.java index e545d45b..30b1921c 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/ContinuingInputStream.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/ContinuingInputStream.java @@ -93,10 +93,13 @@ public void continueWith(final InputStream next) { throw new IllegalStateException("Stream is closed, refusing to set new source"); } - // QUESTION: is the following useful? it signals that the caller has probably done something silly - // if (this.wrapped == next) { - // throw new IllegalArgumentException("Current and next stream are the same"); - // } + if (this.wrapped == next) { + throw new IllegalArgumentException("Current and next stream are the same"); + } + + if (this.wrapped != null) { + this.closeAndDiscardWrapped(); + } this.wrapped = next; } @@ -118,20 +121,15 @@ public int read() throws IOException { return EOF; } - try { - final int b = this.wrapped.read(); + final int b = this.wrapped.read(); - if (b != EOF) { - this.bytesRead += b; - } else { - this.eofSeen = true; - } - - return b; - } catch (final IOException ioe) { - discardWrapped(); - throw ioe; + if (b != EOF) { + this.bytesRead += b; + } else { + this.eofSeen = true; } + + return b; } @Override @@ -142,20 +140,15 @@ public int read(final byte[] b) throws IOException { return EOF; } - try { - final int n = this.wrapped.read(b); - - if (n != EOF) { - this.bytesRead += n; - } else { - this.eofSeen = true; - } + final int n = this.wrapped.read(b); - return n; - } catch (final IOException ioe) { - discardWrapped(); - throw ioe; + if (n != EOF) { + this.bytesRead += n; + } else { + this.eofSeen = true; } + + return n; } @Override @@ -166,50 +159,35 @@ public int read(final byte[] b, final int off, final int len) throws IOException return EOF; } - try { - final int n = this.wrapped.read(b, off, len); - - if (n != EOF) { - this.bytesRead += n; - } else { - this.eofSeen = true; - } + final int n = this.wrapped.read(b, off, len); - return n; - } catch (final IOException ioe) { - discardWrapped(); - throw ioe; + if (n != EOF) { + this.bytesRead += n; + } else { + this.eofSeen = true; } + + return n; } @Override public long skip(final long n) throws IOException { ensureReady(); - try { - final long s = this.wrapped.skip(n); + final long s = this.wrapped.skip(n); - if (0 < s) { - this.bytesRead += s; - } - - return s; - } catch (final IOException ioe) { - discardWrapped(); - throw ioe; + if (0 < s) { + this.bytesRead += s; } + + return s; } @Override public int available() throws IOException { ensureReady(); - try { - return this.wrapped.available(); - } catch (final IOException ioe) { - discardWrapped(); - throw ioe; - } + return this.wrapped.available(); } @Override @@ -252,7 +230,7 @@ private void ensureReady() { } @SuppressWarnings("checkstyle:JavadocMethod") - private void discardWrapped() { + private void closeAndDiscardWrapped() { IOUtils.closeQuietly(this.wrapped); this.wrapped = null; } From 402cc376c20fb14aa6152559139ce5003ee7f4fc Mon Sep 17 00:00:00 2001 From: Tomas Celaya Date: Thu, 2 Aug 2018 13:54:27 -0700 Subject: [PATCH 2/8] Clean up debug log --- .../ApacheHttpGetResponseEntityContentContinuator.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java index fde985e5..0b0b8b91 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java @@ -232,10 +232,15 @@ public InputStream buildContinuation(final IOException ex, final long bytesRead) ex); } - LOG.debug("Attempting to build a continuation for request {} to recover at byte offset {} from exception {}", + LOG.debug("Attempting to build a continuation for " + + "[{}] request " + + "to path [{}] " + + "to recover at byte offset {} " + + "from exception {}", + this.request.getMethod(), this.request.getRequestLine(), bytesRead, - ex); + ex.getMessage()); // if an IOException occurs while reading EOF the user may ask us for a continuation // starting after the last valid byte. From d2d62d012448baa8bf2cc574eb97a3dfe47d0841 Mon Sep 17 00:00:00 2001 From: Tomas Celaya Date: Thu, 2 Aug 2018 14:01:48 -0700 Subject: [PATCH 3/8] Stop expecting ContinuingInputStream to automatically close the errored InputStream --- .../http/ApacheHttpGetResponseEntityContentContinuator.java | 2 +- .../java/com/joyent/manta/util/ContinuingInputStreamTest.java | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java index 0b0b8b91..6d45c9c9 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java @@ -238,7 +238,7 @@ public InputStream buildContinuation(final IOException ex, final long bytesRead) + "to recover at byte offset {} " + "from exception {}", this.request.getMethod(), - this.request.getRequestLine(), + this.request.getRequestLine().getUri(), bytesRead, ex.getMessage()); diff --git a/java-manta-client-unshaded/src/test/java/com/joyent/manta/util/ContinuingInputStreamTest.java b/java-manta-client-unshaded/src/test/java/com/joyent/manta/util/ContinuingInputStreamTest.java index 52c6a888..c48ee5a1 100644 --- a/java-manta-client-unshaded/src/test/java/com/joyent/manta/util/ContinuingInputStreamTest.java +++ b/java-manta-client-unshaded/src/test/java/com/joyent/manta/util/ContinuingInputStreamTest.java @@ -368,8 +368,6 @@ public void testAvailableCanAlsoThrow() throws IOException { final ContinuingInputStream cis = new ContinuingInputStream(new BrokenInputStream()); assertThrows(IOException.class, () -> cis.available()); - - assertThrows(IllegalStateException.class, () -> cis.read()); } public void testSkipCompletelySingleOperation() throws IOException { @@ -433,8 +431,6 @@ public void testFailureFromSkipThenRead() throws IOException { assertThrows(IOException.class, () -> cis.skip(1)); assertEquals(cis.getBytesRead(), 0); - assertThrows(IllegalStateException.class, () -> cis.skip(1)); - cis.continueWith(new ByteArrayInputStream(STUB_OBJECT_BYTES)); assertArrayEquals(STUB_OBJECT_BYTES, IOUtils.toByteArray(cis)); From 7f95792b1536b6e3d04571b3b75f96baaeb0dab9 Mon Sep 17 00:00:00 2001 From: Tomas Celaya Date: Thu, 2 Aug 2018 14:35:13 -0700 Subject: [PATCH 4/8] Add a test case verifying that AutoContinuingInputStream does not complicate the fatal exception --- .../manta/util/AutoContinuingInputStream.java | 6 +- .../util/AutoContinuingInputStreamTest.java | 66 +++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 java-manta-client-unshaded/src/test/java/com/joyent/manta/util/AutoContinuingInputStreamTest.java diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java index d256054d..fe98e0e1 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java @@ -143,8 +143,10 @@ public void close() throws IOException { final InputStream wrapped = this.getWrapped(); - if (wrapped != null) { - wrapped.close(); + if (wrapped == null) { + return; } + + wrapped.close(); } } diff --git a/java-manta-client-unshaded/src/test/java/com/joyent/manta/util/AutoContinuingInputStreamTest.java b/java-manta-client-unshaded/src/test/java/com/joyent/manta/util/AutoContinuingInputStreamTest.java new file mode 100644 index 00000000..4b3e43fe --- /dev/null +++ b/java-manta-client-unshaded/src/test/java/com/joyent/manta/util/AutoContinuingInputStreamTest.java @@ -0,0 +1,66 @@ +package com.joyent.manta.util; + +import org.apache.commons.io.IOUtils; +import org.apache.commons.io.input.ClosedInputStream; +import org.apache.commons.io.input.ProxyInputStream; +import org.testng.annotations.Test; + +import java.io.IOException; +import java.io.InputStream; + +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.same; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertSame; +import static org.testng.Assert.expectThrows; + +@Test +public class AutoContinuingInputStreamTest { + + /** + * We can't use {@link org.apache.commons.io.input.BrokenInputStream} for this test because it will return the same + * exception during close and trigger the + * self-suppression + * issue. + *

+ * On the other hand, we can't use {@link FailingInputStream} because that generates its own exceptions, so we + * wouldn't be able to use {@link org.testng.Assert#assertSame} and check that no suppressed exceptions were added. + */ + private static final class ReadExceptionInputStream extends ProxyInputStream { + + private final IOException exception; + + public ReadExceptionInputStream(final IOException exception) { + super(ClosedInputStream.CLOSED_INPUT_STREAM); + this.exception = exception; + } + + @Override + protected void beforeRead(final int n) throws IOException { + throw this.exception; + } + } + + public void rethrowsUnrecoverableExceptionsDirectly() throws Exception { + // the exception to consider fatal + final IOException ex = new IOException("oops"); + + // source stream always throws that exception + final InputStream original = new ReadExceptionInputStream(ex); + + // pretend that it was a fatal exception and should be rethrown + final InputStreamContinuator continuator = mock(InputStreamContinuator.class); + when(continuator.buildContinuation(same(ex), anyLong())).thenThrow(ex); + + final IOException caught = expectThrows(IOException.class, () -> { + try (final AutoContinuingInputStream in = new AutoContinuingInputStream(original, continuator)) { + IOUtils.toByteArray(in); + } + }); + + assertSame(caught, ex); + assertEquals(caught.getSuppressed().length, 0); + } +} From 2e62f41aa65e8e0d624e26f4035eb91c07cec75d Mon Sep 17 00:00:00 2001 From: Tomas Celaya Date: Thu, 2 Aug 2018 14:56:26 -0700 Subject: [PATCH 5/8] It's fine to add other recovery exceptions as suppressed exceptions, but don't be stupid about adding an exception to itself --- .../joyent/manta/util/AutoContinuingInputStream.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java index fe98e0e1..8ded7b33 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java @@ -57,12 +57,17 @@ public AutoContinuingInputStream(final InputStream wrapped, * continuing */ private void attemptRecovery(final IOException originalIOException) throws IOException { - final InputStream continuation; try { - continuation = this.continuator.buildContinuation(originalIOException, this.getBytesRead()); - super.continueWith(continuation); + super.continueWith(this.continuator.buildContinuation(originalIOException, this.getBytesRead())); } catch (final IOException ioe) { LOG.debug("Failed to automatically recover: {}", ioe.getMessage()); + + // if a different exception was thrown while recovering, add it as a suppressed exception + if (originalIOException != ioe) { + originalIOException.addSuppressed(ioe); + } + + // rethrow the original exception throw originalIOException; } } From d9226e3b3efb6536763447f4355f863395391401 Mon Sep 17 00:00:00 2001 From: Tomas Celaya Date: Thu, 2 Aug 2018 16:11:20 -0700 Subject: [PATCH 6/8] Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e469906..f7fc101c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ This project aims to adhere to [Semantic Versioning](http://semver.org/). ## [3.2.3-SNAPSHOT] - Coming soon! ### Fixed - [UnsupportedOperationException when getting a 0 byte file using `MantaClient.getAsInputStream`](https://github.com/joyent/java-manta/issues/408) + - [AutoContinuingInputStream fails to handle fatal exceptions correctly and triggers a self-suppression error.](https://github.com/joyent/java-manta/pull/429) ### Added - Client metrics can now be enabled by selecting a reporting mode with the @@ -16,6 +17,9 @@ This project aims to adhere to [Semantic Versioning](http://semver.org/). - [Timers](http://metrics.dropwizard.io/4.0.0/manual/core.html#timer) per HTTP request method. - [Meters](http://metrics.dropwizard.io/4.0.0/manual/core.html#meter) per exception that occurs during requests. - [`MantaClient#delete(String, MantaHttpHeaders)`](https://github.com/joyent/java-manta/issues/427) + - [Download auto-resume](https://github.com/joyent/java-manta/issues/411) has been added in the form of + [`manta.download_continuations`/`MANTA_DOWNLOAD_CONTINUATIONS` configuration + setting](https://github.com/joyent/java-manta/blob/master/USAGE.md#download-continuation). ### Changed - MBeans registered in JMX no longer use an incrementing integer and instead are created under From 91cc5f5b1d47b21e04c5d16e4e0ff1f3eed512d3 Mon Sep 17 00:00:00 2001 From: Tomas Celaya Date: Fri, 3 Aug 2018 13:31:21 -0700 Subject: [PATCH 7/8] Move ApacheHttpGetResponseEntityContentContinuatorIT to the right package --- .../com/joyent/manta/client/MantaClient.java | 21 +++++++++++++++++++ ...GetResponseEntityContentContinuatorIT.java | 6 ++---- 2 files changed, 23 insertions(+), 4 deletions(-) rename java-manta-it/src/test/java/com/joyent/manta/{client => http}/ApacheHttpGetResponseEntityContentContinuatorIT.java (99%) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaClient.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaClient.java index fc6fd636..959b3fcb 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaClient.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaClient.java @@ -209,6 +209,26 @@ public MantaClient(final ConfigContext config, this(config, connectionFactoryConfigurator, null, null); } + /** + * Creates a new instance of the Manta client based on user-provided connection objects. This allows for a higher + * degree of customization at the cost of more involvement from the consumer. + * + * Users opting into advanced configuration (i.e. not passing {@code null} as the second parameter) + * should be comfortable with the internals of {@link CloseableHttpClient} and accept that we can only make a + * best effort to support all possible use-cases. For example, users may pass in a builder which is wired to a + * {@link org.apache.http.impl.conn.BasicHttpClientConnectionManager} and effectively make the client + * single-threaded by eliminating the connection pool. Bug or feature? You decide! + * + * @param config The configuration context that provides all of the configuration values + * @param connectionFactoryConfigurator pre-configured objects for use with a MantaConnectionFactory (or null) + * @param metricConfiguration the metrics registry and configuration, or null to prepare one from the general config + */ + public MantaClient(final ConfigContext config, + final MantaConnectionFactoryConfigurator connectionFactoryConfigurator, + final MantaClientMetricConfiguration metricConfiguration) { + this(config, connectionFactoryConfigurator, null, metricConfiguration); + } + /** * Creates a new instance of the Manta client based on user-provided connection objects. This allows for a higher * degree of customization at the cost of more involvement from the consumer. @@ -222,6 +242,7 @@ public MantaClient(final ConfigContext config, * @param config The configuration context that provides all of the configuration values * @param connectionFactoryConfigurator pre-configured objects for use with a MantaConnectionFactory (or null) * @param httpHelper helper object for executing http requests (or null to build one ourselves) + * @param metricConfiguration the metrics registry and configuration, or null to prepare one from the general config */ MantaClient(final ConfigContext config, final MantaConnectionFactoryConfigurator connectionFactoryConfigurator, diff --git a/java-manta-it/src/test/java/com/joyent/manta/client/ApacheHttpGetResponseEntityContentContinuatorIT.java b/java-manta-it/src/test/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuatorIT.java similarity index 99% rename from java-manta-it/src/test/java/com/joyent/manta/client/ApacheHttpGetResponseEntityContentContinuatorIT.java rename to java-manta-it/src/test/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuatorIT.java index 310ab4e4..771c0c5a 100644 --- a/java-manta-it/src/test/java/com/joyent/manta/client/ApacheHttpGetResponseEntityContentContinuatorIT.java +++ b/java-manta-it/src/test/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuatorIT.java @@ -1,8 +1,9 @@ -package com.joyent.manta.client; +package com.joyent.manta.http; import com.codahale.metrics.Counter; import com.codahale.metrics.MetricFilter; import com.codahale.metrics.MetricRegistry; +import com.joyent.manta.client.MantaClient; import com.joyent.manta.client.crypto.AesCtrCipherDetails; import com.joyent.manta.client.crypto.SecretKeyUtils; import com.joyent.manta.client.crypto.SupportedCipherDetails; @@ -13,8 +14,6 @@ import com.joyent.manta.config.IntegrationTestConfigContext; import com.joyent.manta.config.MantaClientMetricConfiguration; import com.joyent.manta.config.MetricReporterMode; -import com.joyent.manta.http.HttpRange; -import com.joyent.manta.http.MantaHttpHeaders; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.StringUtils; @@ -392,7 +391,6 @@ private MantaClient prepareClient(final SupportedCipherDetails cipherDetails, } final MantaClient mantaClient = new MantaClient(config, - null, null, metricConfig); From e0bb5e27f5a961a13b8f0f7f2c645043ba5f56af Mon Sep 17 00:00:00 2001 From: Tomas Celaya Date: Fri, 3 Aug 2018 14:05:40 -0700 Subject: [PATCH 8/8] PR suggestions --- .../http/ApacheHttpGetResponseEntityContentContinuator.java | 2 ++ .../java/com/joyent/manta/util/AutoContinuingInputStream.java | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java index 6d45c9c9..9fb7c5b6 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/http/ApacheHttpGetResponseEntityContentContinuator.java @@ -218,6 +218,8 @@ public class ApacheHttpGetResponseEntityContentContinuator implements InputStrea */ @Override public InputStream buildContinuation(final IOException ex, final long bytesRead) throws IOException { + requireNonNull(ex); + if (!isRecoverable(ex)) { throw ex; } diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java index 8ded7b33..0171a7ca 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/util/AutoContinuingInputStream.java @@ -13,6 +13,7 @@ import java.io.IOException; import java.io.InputStream; +import java.io.UncheckedIOException; import static java.util.Objects.requireNonNull; @@ -59,7 +60,7 @@ public AutoContinuingInputStream(final InputStream wrapped, private void attemptRecovery(final IOException originalIOException) throws IOException { try { super.continueWith(this.continuator.buildContinuation(originalIOException, this.getBytesRead())); - } catch (final IOException ioe) { + } catch (final UncheckedIOException | IOException ioe) { LOG.debug("Failed to automatically recover: {}", ioe.getMessage()); // if a different exception was thrown while recovering, add it as a suppressed exception