Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: connection closed prematurely in BlobReadChannel & ConnectionReset #173

Merged
merged 10 commits into from Mar 13, 2020
Merged

fix: connection closed prematurely in BlobReadChannel & ConnectionReset #173

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3803ac9
fix: connection closed prematurely
frankyn Mar 11, 2020
0baa6aa
add connection reset retry
frankyn Mar 11, 2020
9c01f15
remove integration test
frankyn Mar 11, 2020
c5938b9
format
frankyn Mar 11, 2020
1246212
clean up tests
frankyn Mar 11, 2020
42d95a4
change design of transient retry strategy
frankyn Mar 12, 2020
fb31431
add documentation
frankyn Mar 12, 2020
ffd7b1f
format docs
frankyn Mar 12, 2020
5663daf
address feedback
frankyn Mar 12, 2020
e918305
address feedback
frankyn Mar 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 31 additions & 1 deletion google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java
View file
Open in desktop
Expand Up @@ -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 =
Expand All @@ -43,7 +46,9 @@ public final class StorageException extends BaseHttpServiceException {
new Error(500, null),
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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...

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Collaborator

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

Copy link
Contributor

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. :-(

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;

Expand All @@ -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);
}
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaults

* idempotent always being {@code True}. This method will force retry of the following transient
Copy link
Contributor

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

* issues (Connection Closed Prematurely, Connection Reset).
Copy link
Contributor

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

*
* <p>Please Review {@code RETRYABLE_ERRORS} for a full list of retryable errors.
Copy link
Contributor

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

*
* @throws StorageException when {@code ex} was caused by a {@code StorageException}
Copy link
Contributor

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

*/
public static StorageException translate(IOException exception) {
crwilcox marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

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?

Copy link
Member Author

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

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);
}
}
}
2 changes: 1 addition & 1 deletion google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
View file
Open in desktop
Expand Up @@ -701,7 +701,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]);
}
Expand Down
2 changes: 0 additions & 2 deletions google-cloud-storage/src/test/java/com/google/cloud/storage/BlobReadChannelTest.java
View file
Open in desktop
Expand Up @@ -28,7 +28,6 @@

import com.google.cloud.ReadChannel;
import com.google.cloud.RestorableState;
import com.google.cloud.ServiceOptions;
import com.google.cloud.Tuple;
import com.google.cloud.storage.spi.StorageRpcFactory;
import com.google.cloud.storage.spi.v1.StorageRpc;
Expand Down Expand Up @@ -68,7 +67,6 @@ public void setUp() {
StorageOptions.newBuilder()
.setProjectId("projectId")
.setServiceRpcFactory(rpcFactoryMock)
.setRetrySettings(ServiceOptions.getNoRetrySettings())
.build();
}

Expand Down
25 changes: 25 additions & 0 deletions google-cloud-storage/src/test/java/com/google/cloud/storage/StorageExceptionTest.java
View file
Open in desktop
Expand Up @@ -32,7 +32,9 @@
import com.google.cloud.BaseServiceException;
import com.google.cloud.RetryHelper.RetryHelperException;
import java.io.IOException;
import java.net.SocketException;
import java.net.SocketTimeoutException;
import javax.net.ssl.SSLException;
import org.junit.Test;

public class StorageExceptionTest {
Expand Down Expand Up @@ -135,6 +137,29 @@ public void testStorageException() {
assertTrue(exception.isRetryable());
}

@Test
public void testTranslateConnectionReset() {
StorageException exception =
StorageException.translate(
new IOException(
"Connection has been shutdown: "
+ new SSLException(new SocketException("Connection reset"))));
assertEquals(0, exception.getCode());
assertEquals("connectionReset", exception.getReason());
assertTrue(exception.isRetryable());
}

@Test
public void testTranslateConnectionClosedPrematurely() {
StorageException exception =
StorageException.translate(
new IOException(
"Connection closed prematurely: bytesRead = 1114112, Content-Length = 10485760"));
assertEquals(0, exception.getCode());
assertEquals("connectionClosedPrematurely", exception.getReason());
assertTrue(exception.isRetryable());
}

@Test
public void testTranslateAndThrow() throws Exception {
Exception cause = new StorageException(503, "message");
Expand Down