fix: connection closed prematurely in BlobReadChannel & ConnectionReset #173

frankyn · 2020-03-11T02:51:51Z

Fixes #165

Introduce:

public StorageException(int code, String message, String reason, Throwable cause):
to capture the reason which is used to check RETRYABLE_ERRORS.
public static StorageException translate(IOException exception): to help translate idempotent request exceptions in case they're RETRYABLE_ERRORS. This translate method should be introduced for other idempotent requests, but that's out of scope for this request.

codecov · 2020-03-11T02:55:19Z

Codecov Report

Merging #173 into master will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master     #173      +/-   ##
============================================
+ Coverage     63.46%   63.48%   +0.01%     
- Complexity      537      540       +3     
============================================
  Files            30       30              
  Lines          4752     4760       +8     
  Branches        427      428       +1     
============================================
+ Hits           3016     3022       +6     
- Misses         1576     1577       +1     
- Partials        160      161       +1

Impacted Files	Coverage Δ	Complexity Δ
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.63% <0%> (ø)`	`1 <0> (ø)`	⬇️
...ava/com/google/cloud/storage/StorageException.java	`92.3% <75%> (-7.7%)`	`9 <3> (+3)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1617c40...e918305. Read the comment docs.

elharo · 2020-03-11T17:40:29Z

google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

+      } else if (serviceException.getMessage().contains("Connection closed prematurely")) {
+        serviceException = new StorageException(new SocketException(serviceException.getMessage()));
+      } else if (serviceException.getMessage().contains("Connection reset")) {
+        serviceException = new StorageException(new SocketException(serviceException.getMessage()));


This loses the stack trace; you should include the servieException too

elharo · 2020-03-11T17:41:12Z

google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

@@ -704,6 +705,10 @@ public long read(
      StorageException serviceException = translate(ex);
      if (serviceException.getCode() == SC_REQUESTED_RANGE_NOT_SATISFIABLE) {
        return Tuple.of(null, new byte[0]);
+      } else if (serviceException.getMessage().contains("Connection closed prematurely")) {
+        serviceException = new StorageException(new SocketException(serviceException.getMessage()));


This is very weird. I don't think I've ever seen this double wrapping in one throw clause done before. Can you explain the purpose here and how it works?

elharo · 2020-03-11T17:41:59Z

google-cloud-storage/src/test/java/com/google/cloud/storage/BlobReadChannelTest.java

@@ -84,6 +84,85 @@ public void testCreate() {
    assertTrue(reader.isOpen());
  }

+  @Test
+  public void testCreateRetryableErrorPrematureClosure() throws IOException {
+    byte[] arr = {0x0, 0xd, 0xa};


array (no abbreviations per Google style)

elharo · 2020-03-11T17:42:20Z

google-cloud-storage/src/test/java/com/google/cloud/storage/BlobReadChannelTest.java

+
+  @Test
+  public void testCreateNonRetryableErrorPrematureClosure() throws IOException {
+    byte[] arr = {0x0, 0xd, 0xa};


elharo · 2020-03-11T17:42:38Z

google-cloud-storage/src/test/java/com/google/cloud/storage/BlobReadChannelTest.java

+    expect(storageRpcMock.read(BLOB_ID.toPb(), EMPTY_RPC_OPTIONS, 0, 2097152)).andThrow(exception);
+    replay(storageRpcMock);
+    try {
+      assertTrue(reader.read(byteBuffer) > 0);


don't both assertTrue and fail

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

crwilcox · 2020-03-12T00:21:58Z

google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

@@ -226,6 +226,7 @@ public void onFailure(GoogleJsonError googleJsonError, HttpHeaders httpHeaders)
  }

  private static StorageException translate(IOException exception) {
+


Do we run a formatter for java? Is this blank line a problem for it?

It doesn't clean it up surprisingly.

elharo · 2020-03-12T15:50:38Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

@@ -43,7 +46,9 @@
          new Error(500, null),


Where does this Error class come from?

https://github.com/googleapis/java-core/blob/master/google-cloud-core/src/main/java/com/google/cloud/BaseServiceException.java#L174

If this had not been static imported, I probably could have figured that out...

Also that's marked @InternalApi so it shouldn't be used here at all.

StorageException is also annotated with @InternalApi. I'm not introducing the use Error(). I think this is okay, as Storage isn't the only library using the base class BaseHttpServiceException, e.g. Bigquery.

For @InternalApi from google-cloud-core, it's public because it needs to be, but intended for use by our libraries

I'll live with it this PR, but that's a major abuse of @InternalApi. It's as effectively locked in as any other public API. We should admit what's been done here, and remove @InternalApi from those classes. We can't change them absent an incompatible major version bump. :-(

elharo · 2020-03-12T15:51:21Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

@@ -73,4 +82,25 @@ public static StorageException translateAndThrow(RetryHelperException ex) {
    BaseServiceException.translate(ex);
    throw new StorageException(UNKNOWN_CODE, ex.getMessage(), ex.getCause());
  }
+
+  /**
+   * Translate IOException to the StorageException that caused the error. This method default to


elharo · 2020-03-12T15:51:32Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

+
+  /**
+   * Translate IOException to the StorageException that caused the error. This method default to
+   * idempotent always being {@code True}. This method will force retry of the following transient


true? (primitive)
will force --> forces
or perhaps just will force retry of --> retries

elharo · 2020-03-12T15:52:51Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

+  /**
+   * Translate IOException to the StorageException that caused the error. This method default to
+   * idempotent always being {@code True}. This method will force retry of the following transient
+   * issues (Connection Closed Prematurely, Connection Reset).


"following" isn't needed; remove parentheses

elharo · 2020-03-12T15:53:26Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

+   * idempotent always being {@code True}. This method will force retry of the following transient
+   * issues (Connection Closed Prematurely, Connection Reset).
+   *
+   * <p>Please Review {@code RETRYABLE_ERRORS} for a full list of retryable errors.


rewrite without "please Review"
also, never use Please in technical docs per https://developers.google.com/style/tone

elharo · 2020-03-12T15:55:12Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

+   *
+   * <p>Please Review {@code RETRYABLE_ERRORS} for a full list of retryable errors.
+   *
+   * @throws StorageException when {@code ex} was caused by a {@code StorageException}


It returns, it doesn't throw

frankyn · 2020-03-12T16:16:14Z

Thanks @elharo, I addressed your comments.

elharo · 2020-03-12T17:59:58Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

@@ -43,7 +46,9 @@
          new Error(500, null),


If this had not been static imported, I probably could have figured that out...

elharo · 2020-03-12T18:01:10Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

@@ -43,7 +46,9 @@
          new Error(500, null),


Also that's marked @InternalApi so it shouldn't be used here at all.

elharo · 2020-03-12T18:06:48Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

+
+  /**
+   * Translate IOException to the StorageException that caused the error. This method defaults to
+   * idempotent always being {@code true}. This method retries the transient issues Connection


I don't think it retries anything. There's no network connection here.

Ah,,, that's not worded well fixing.

elharo · 2020-03-12T18:08:04Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

@@ -73,4 +82,25 @@ public static StorageException translateAndThrow(RetryHelperException ex) {
    BaseServiceException.translate(ex);
    throw new StorageException(UNKNOWN_CODE, ex.getMessage(), ex.getCause());
  }
+
+  /**
+   * Translate IOException to the StorageException that caused the error. This method defaults to


to the... --> to a StorageException representing the cause of the error. (otherwise you have the effect coming before the cause.)

elharo · 2020-03-12T18:09:05Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

+   * idempotent always being {@code true}. This method retries the transient issues Connection
+   * Closed Prematurely and Connection Reset.
+   *
+   * <p>Review {@code RETRYABLE_ERRORS} for a full list of retryable errors.


RETRYABLE_ERRORS is rightly private, but it won't show in API docs.

elharo · 2020-03-12T18:09:43Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java

+   *
+   * @returns {@code StorageException}
+   */
+  public static StorageException translate(IOException exception) {


translate is an uncommon name for this operation. Perhaps wrap?

Translate is what's being used in the code right now and wanted to keep consistent given there's already translateAndThrow

frankyn · 2020-03-13T17:18:28Z

Merging after discussion with @elharo offline. Two approvals is more than enough for this.

🤖 I have created a release \*beep\* \*boop\* --- ### [1.105.2](https://www.github.com/googleapis/java-storage/compare/v1.105.1...v1.105.2) (2020-03-13) ### Bug Fixes * connection closed prematurely in BlobReadChannel & ConnectionReset ([#173](https://www.github.com/googleapis/java-storage/issues/173)) ([27bccda](https://www.github.com/googleapis/java-storage/commit/27bccda384da4a7b877b371fbaecc794d6304fbf)) ### Dependencies * update core dependencies ([#171](https://www.github.com/googleapis/java-storage/issues/171)) ([ef5f2c6](https://www.github.com/googleapis/java-storage/commit/ef5f2c6e5079debe8f7f37c3d2c501aac3dc82a6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).

fix: connection closed prematurely

3803ac9

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 11, 2020

frankyn requested review from chingor13 and JesseLovelace March 11, 2020 02:52

frankyn added 3 commits March 10, 2020 22:18

add connection reset retry

0baa6aa

remove integration test

9c01f15

format

c5938b9

frankyn changed the title ~~fix: connection closed prematurely in BlobReadChannel~~ fix: connection closed prematurely in BlobReadChannel & ConnectionReset Mar 11, 2020

frankyn requested a review from elharo March 11, 2020 16:35

clean up tests

1246212

elharo reviewed Mar 11, 2020

View reviewed changes

change design of transient retry strategy

42d95a4

frankyn force-pushed the fix-premature-closure branch from 47fc04c to 42d95a4 Compare March 12, 2020 00:15

crwilcox reviewed Mar 12, 2020

View reviewed changes

frankyn added 2 commits March 11, 2020 17:26

add documentation

fb31431

format docs

ffd7b1f

frankyn requested review from elharo and crwilcox and removed request for chingor13 March 12, 2020 00:36

crwilcox approved these changes Mar 12, 2020

View reviewed changes

frankyn requested a review from chingor13 March 12, 2020 00:55

elharo reviewed Mar 12, 2020

View reviewed changes

address feedback

5663daf

frankyn requested a review from elharo March 12, 2020 16:18

JesseLovelace approved these changes Mar 12, 2020

View reviewed changes

elharo reviewed Mar 12, 2020

View reviewed changes

address feedback

e918305

frankyn requested a review from elharo March 12, 2020 18:23

frankyn merged commit 27bccda into master Mar 13, 2020

frankyn deleted the fix-premature-closure branch March 13, 2020 17:18

original-brownbear mentioned this pull request Mar 24, 2020

Upgrade GCS Dependency to 1.106.0 elastic/elasticsearch#54092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: connection closed prematurely in BlobReadChannel & ConnectionReset #173

fix: connection closed prematurely in BlobReadChannel & ConnectionReset #173

frankyn commented Mar 11, 2020 •

edited

codecov bot commented Mar 11, 2020 •

edited

elharo Mar 11, 2020

elharo Mar 11, 2020

elharo Mar 11, 2020

elharo Mar 11, 2020

elharo Mar 11, 2020

crwilcox Mar 12, 2020

frankyn Mar 12, 2020

elharo Mar 12, 2020

frankyn Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

frankyn Mar 12, 2020

chingor13 Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

frankyn commented Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

frankyn Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

elharo Mar 12, 2020

frankyn Mar 12, 2020

frankyn commented Mar 13, 2020

		@@ -226,6 +226,7 @@ public void onFailure(GoogleJsonError googleJsonError, HttpHeaders httpHeaders)
		}

		private static StorageException translate(IOException exception) {

fix: connection closed prematurely in BlobReadChannel & ConnectionReset #173

fix: connection closed prematurely in BlobReadChannel & ConnectionReset #173

Conversation

frankyn commented Mar 11, 2020 • edited

codecov bot commented Mar 11, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn commented Mar 13, 2020

frankyn commented Mar 11, 2020 •

edited

codecov bot commented Mar 11, 2020 •

edited