-
Notifications
You must be signed in to change notification settings - Fork 80
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
fix: connection closed prematurely in BlobReadChannel & ConnectionReset #173
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
} 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())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loses the stack trace; you should include the servieException too
@@ -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())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -84,6 +84,85 @@ public void testCreate() { | |||
assertTrue(reader.isOpen()); | |||
} | |||
|
|||
@Test | |||
public void testCreateRetryableErrorPrematureClosure() throws IOException { | |||
byte[] arr = {0x0, 0xd, 0xa}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array (no abbreviations per Google style)
|
||
@Test | ||
public void testCreateNonRetryableErrorPrematureClosure() throws IOException { | ||
byte[] arr = {0x0, 0xd, 0xa}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array
expect(storageRpcMock.read(BLOB_ID.toPb(), EMPTY_RPC_OPTIONS, 0, 2097152)).andThrow(exception); | ||
replay(storageRpcMock); | ||
try { | ||
assertTrue(reader.read(byteBuffer) > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't both assertTrue and fail
47fc04c
to
42d95a4
Compare
google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java
Show resolved
Hide resolved
@@ -226,6 +226,7 @@ public void onFailure(GoogleJsonError googleJsonError, HttpHeaders httpHeaders) | |||
} | |||
|
|||
private static StorageException translate(IOException exception) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we run a formatter for java? Is this blank line a problem for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't clean it up surprisingly.
@@ -43,7 +46,9 @@ | |||
new Error(500, null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this Error class come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this had not been static imported, I probably could have figured that out...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also that's marked @InternalApi
so it shouldn't be used here at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For @InternalApi
from google-cloud-core, it's public because it needs to be, but intended for use by our libraries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :-(
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults
|
||
/** | ||
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true? (primitive)
will force --> forces
or perhaps just will force retry of --> retries
/** | ||
* 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"following" isn't needed; remove parentheses
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rewrite without "please Review"
also, never use Please in technical docs per https://developers.google.com/style/tone
* | ||
* <p>Please Review {@code RETRYABLE_ERRORS} for a full list of retryable errors. | ||
* | ||
* @throws StorageException when {@code ex} was caused by a {@code StorageException} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns, it doesn't throw
Thanks @elharo, I addressed your comments. |
@@ -43,7 +46,9 @@ | |||
new Error(500, null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this had not been static imported, I probably could have figured that out...
@@ -43,7 +46,9 @@ | |||
new Error(500, null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also that's marked @InternalApi
so it shouldn't be used here at all.
|
||
/** | ||
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it retries anything. There's no network connection here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah,,, that's not worded well fixing.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to the... --> to a StorageException representing the cause of the error. (otherwise you have the effect coming before the cause.)
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RETRYABLE_ERRORS is rightly private, but it won't show in API docs.
* | ||
* @returns {@code StorageException} | ||
*/ | ||
public static StorageException translate(IOException exception) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
translate
is an uncommon name for this operation. Perhaps wrap
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translate is what's being used in the code right now and wanted to keep consistent given there's already translateAndThrow
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).
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.