-
Notifications
You must be signed in to change notification settings - Fork 549
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
[S3] Fix issue #616: fix the error handling for transfers when network disconnects #865
Conversation
if (TransferNetworkLossHandler.getInstance() != null && | ||
!TransferNetworkLossHandler.getInstance().isNetworkConnected()) { | ||
LOGGER.info("Network not connected. Setting the state to WAITING_FOR_NETWORK."); | ||
updater.updateState(upload.id, TransferState.WAITING_FOR_NETWORK); |
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.
The status shouldn't be updated here, but instead done by the caller by looking at the return value of the function.
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.
Done.
*/ | ||
private void completeMultiPartUpload(int mainUploadId, String bucket, | ||
private CompleteMultipartUploadResult completeMultiPartUpload(int mainUploadId, String bucket, |
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.
add the " throws AmazonClientException, AmazonServiceException" to this private method.
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.
Done.
updater.updateProgress(upload.id, upload.bytesTotal, upload.bytesTotal, true); | ||
updater.updateState(upload.id, TransferState.COMPLETED); | ||
return true; | ||
if (result != 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.
completeMultiPartUpload throws the AmazonClientException/AmazonServerException if there is an error. The check for result != null is redundant and the else clause would be dead code
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.
Done.
...oid-sdk-s3/src/main/java/com/amazonaws/mobileconnectors/s3/transferutility/DownloadTask.java
Show resolved
Hide resolved
...oid-sdk-s3/src/main/java/com/amazonaws/mobileconnectors/s3/transferutility/DownloadTask.java
Show resolved
Hide resolved
...droid-sdk-s3/src/main/java/com/amazonaws/mobileconnectors/s3/transferutility/UploadTask.java
Show resolved
Hide resolved
...droid-sdk-s3/src/main/java/com/amazonaws/mobileconnectors/s3/transferutility/UploadTask.java
Show resolved
Hide resolved
7770f98
to
2a7f510
Compare
try { | ||
if (TransferNetworkLossHandler.getInstance() != null && | ||
!TransferNetworkLossHandler.getInstance().isNetworkConnected()) { | ||
LOGGER.info("Network not connected. Setting the state to WAITING_FOR_NETWORK."); |
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.
Remove this log statement.
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.
Removed.
2a7f510
to
5ef1b45
Compare
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.
Make sure to catch exceptions thrown by the abort call to ensure that it doesn't crash the app.
@@ -241,6 +242,8 @@ private Boolean uploadMultipartAndWaitForCompletion() throws ExecutionException | |||
} catch (final AmazonClientException ace) { | |||
LOGGER.error("Failed to complete multipart: " + upload.id | |||
+ " due to " + ace.getMessage(), ace); | |||
abortMultiPartUpload(upload.id, upload.bucketName, upload.key, |
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.
Make sure to catch any exceptions thrown by abort as well.
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.
The abortMultiPartUpload
method catches all the exceptions thrown by s3. abortMultiPartUpload
.
…when network disconnects (aws-amplify#865)
Issue #, if available:
#616
Description of changes:
fix the error handling for transfers when network disconnects
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.