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
Changes from 6 commits
3803ac9
0baa6aa
9c01f15
c5938b9
1246212
42d95a4
fb31431
ffd7b1f
5663daf
e918305
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,9 @@ | |
*/ | ||
@InternalApi | ||
public final class StorageException extends BaseHttpServiceException { | ||
private static final String INTERNAL_ERROR = "internalError"; | ||
private static final String CONNECTION_CLOSED_PREMATURELY = "connectionClosedPrematurely"; | ||
private static final String CONNECTION_RESET = "connectionReset"; | ||
|
||
// see: https://cloud.google.com/storage/docs/resumable-uploads-xml#practices | ||
private static final Set<Error> RETRYABLE_ERRORS = | ||
|
@@ -43,7 +46,9 @@ public final class StorageException extends BaseHttpServiceException { | |
new Error(500, null), | ||
new Error(429, null), | ||
new Error(408, null), | ||
new Error(null, "internalError")); | ||
new Error(null, INTERNAL_ERROR), | ||
new Error(null, CONNECTION_CLOSED_PREMATURELY), | ||
new Error(null, CONNECTION_RESET)); | ||
|
||
private static final long serialVersionUID = -4168430271327813063L; | ||
|
||
|
@@ -55,6 +60,10 @@ public StorageException(int code, String message, Throwable cause) { | |
super(code, message, null, true, RETRYABLE_ERRORS, cause); | ||
} | ||
|
||
public StorageException(int code, String message, String reason, Throwable cause) { | ||
super(code, message, reason, true, RETRYABLE_ERRORS, cause); | ||
} | ||
|
||
public StorageException(IOException exception) { | ||
super(exception, true, RETRYABLE_ERRORS); | ||
} | ||
|
@@ -73,4 +82,16 @@ public static StorageException translateAndThrow(RetryHelperException ex) { | |
BaseServiceException.translate(ex); | ||
throw new StorageException(UNKNOWN_CODE, ex.getMessage(), ex.getCause()); | ||
} | ||
|
||
public static StorageException translate(IOException exception) { | ||
crwilcox marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
if (exception.getMessage().contains("Connection closed prematurely")) { | ||
return new StorageException( | ||
0, exception.getMessage(), CONNECTION_CLOSED_PREMATURELY, exception); | ||
} else if (exception.getMessage().contains("Connection reset")) { | ||
return new StorageException(0, exception.getMessage(), CONNECTION_RESET, exception); | ||
} else { | ||
// default | ||
return new StorageException(exception); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't clean it up surprisingly. |
||
return new StorageException(exception); | ||
} | ||
|
||
|
@@ -701,7 +702,7 @@ public Tuple<String, byte[]> read( | |
return Tuple.of(etag, output.toByteArray()); | ||
} catch (IOException ex) { | ||
span.setStatus(Status.UNKNOWN.withDescription(ex.getMessage())); | ||
StorageException serviceException = translate(ex); | ||
StorageException serviceException = StorageException.translate(ex); | ||
if (serviceException.getCode() == SC_REQUESTED_RANGE_NOT_SATISFIABLE) { | ||
return Tuple.of(null, new byte[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.
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.
https://github.com/googleapis/java-core/blob/master/google-cloud-core/src/main/java/com/google/cloud/BaseServiceException.java#L174
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 classBaseHttpServiceException
, 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 librariesThere 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. :-(