fix: connection closed prematurely in BlobReadChannel & ConnectionReset by frankyn · Pull Request #173 · googleapis/java-storage

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

      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


+  @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

  }

  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 @@ public final class StorageException extends BaseHttpServiceException {
          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

  }
+
+  /**
+   * 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 @@ public final class StorageException extends BaseHttpServiceException {
          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 @@ public final class StorageException extends BaseHttpServiceException {
          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

  }
+
+  /**
+   * 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 JesseLovelace and chingor13 March 11, 2020 02:52

Frank Natividad 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

Frank Natividad added 2 commits March 11, 2020 17:26

add documentation

fb31431

format docs

ffd7b1f

frankyn requested review from crwilcox and elharo 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

		}

		private static StorageException translate(IOException exception) {

		@@ -43,7 +46,9 @@ public final class StorageException extends BaseHttpServiceException {
		new Error(500, null),

Conversation

frankyn commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

frankyn commented Mar 12, 2020

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!

frankyn commented Mar 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

frankyn commented Mar 11, 2020 •

edited

Loading

codecov bot commented Mar 11, 2020 •

edited

Loading