Skip to content

Commit 2fbf22c

Browse files
narahavinarahavi
committed
upadating upload/download based on pr comments
1 parent 8d3da65 commit 2fbf22c

7 files changed

Lines changed: 81 additions & 52 deletions

File tree

‎src/aws-cpp-sdk-s3-transfer/include/aws/s3-transfer/DownloadRequest.h‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <aws/s3-transfer/ProgressListener.h>
88
#include <aws/s3-transfer/DownloadDataReceiver.h>
99
#include <aws/core/client/AWSError.h>
10+
#include <aws/crt/Optional.h>
1011
#include <aws/core/utils/DateTime.h>
1112
#include <aws/core/utils/memory/AWSMemory.h>
1213
#include <aws/core/utils/memory/stl/AWSString.h>
@@ -25,6 +26,8 @@ struct DownloadTransferState;
2526

2627
namespace Internal {
2728
class DownloadRequestImpl;
29+
30+
using OptionalError = Aws::Crt::Optional<Aws::Client::AWSError<Aws::S3::S3Errors>>;
2831
}
2932

3033
/**
@@ -74,10 +77,10 @@ class AWS_S3_TRANSFER_API DownloadRequest final {
7477

7578
const Aws::S3::Model::GetObjectRequest& GetS3Request() const;
7679

77-
Aws::Client::AWSError<Aws::S3::S3Errors> Validate() const;
78-
Aws::Client::AWSError<Aws::S3::S3Errors> FinalizeOnSuccess(
80+
Internal::OptionalError Validate() const;
81+
Internal::OptionalError FinalizeOnSuccess(
7982
const std::shared_ptr<DownloadTransferState>& state) const;
80-
void CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const;
83+
Internal::OptionalError CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const;
8184

8285
private:
8386
std::shared_ptr<Internal::DownloadRequestImpl> m_impl;

‎src/aws-cpp-sdk-s3-transfer/include/aws/s3-transfer/UploadRequest.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ struct UploadTransferState;
3131

3232
namespace Internal {
3333
class UploadRequestImpl;
34+
35+
using OptionalError = Aws::Crt::Optional<Aws::Client::AWSError<Aws::S3::S3Errors>>;
3436
}
3537

3638
/**
@@ -104,7 +106,7 @@ class AWS_S3_TRANSFER_API UploadRequest final {
104106

105107
const Aws::S3::Model::PutObjectRequest& GetS3Request() const;
106108

107-
Aws::Client::AWSError<Aws::S3::S3Errors> PrepareTransferState(
109+
Internal::OptionalError PrepareTransferState(
108110
const std::shared_ptr<UploadTransferState>& state) const;
109111

110112
private:

‎src/aws-cpp-sdk-s3-transfer/include/aws/s3-transfer/internal/DownloadRequestImpl.h‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -36,16 +36,16 @@ class AWS_CORE_LOCAL DownloadRequestImpl {
3636
DownloadRequestImpl(DownloadRequestImpl&&) = delete;
3737
DownloadRequestImpl& operator=(DownloadRequestImpl&&) = delete;
3838

39-
virtual Aws::Client::AWSError<Aws::S3::S3Errors> Validate() const {
40-
return Aws::Client::AWSError<Aws::S3::S3Errors>();
41-
}
39+
// Empty when the request is usable; otherwise the validation error.
40+
virtual OptionalError Validate() const { return OptionalError(); }
4241

4342
// Runs once the transfer succeeds; the file destination promotes its temp file here.
44-
virtual Aws::Client::AWSError<Aws::S3::S3Errors> FinalizeOnSuccess(
43+
virtual OptionalError FinalizeOnSuccess(
4544
const std::shared_ptr<DownloadTransferState>& state) const = 0;
4645

47-
// Runs on failure; the file destination removes the temp file the CRT left behind.
48-
virtual void CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const = 0;
46+
// Runs on failure; the file destination removes the temp file the CRT left behind. Empty on
47+
// success; otherwise an error the caller can surface alongside the transfer's own failure.
48+
virtual OptionalError CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const = 0;
4949

5050
// Empty unless this request downloads to a file, in which case the CRT writes the file directly.
5151
virtual const Aws::String& GetDestinationFilePath() const;
@@ -69,10 +69,10 @@ class AWS_CORE_LOCAL FileDownloadImpl final : public DownloadRequestImpl {
6969
Aws::String destinationFilePath,
7070
Aws::Vector<std::shared_ptr<DownloadProgressListener>> transferListeners);
7171

72-
Aws::Client::AWSError<Aws::S3::S3Errors> Validate() const override;
73-
Aws::Client::AWSError<Aws::S3::S3Errors> FinalizeOnSuccess(
72+
OptionalError Validate() const override;
73+
OptionalError FinalizeOnSuccess(
7474
const std::shared_ptr<DownloadTransferState>& state) const override;
75-
void CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const override;
75+
OptionalError CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const override;
7676
const Aws::String& GetDestinationFilePath() const override { return m_destinationFilePath; }
7777
const Aws::String& GetTempFilePath() const override { return m_tempFilePath; }
7878

@@ -88,9 +88,9 @@ class AWS_CORE_LOCAL StreamDownloadImpl final : public DownloadRequestImpl {
8888
std::shared_ptr<DownloadDataReceiver> dataReceiver,
8989
Aws::Vector<std::shared_ptr<DownloadProgressListener>> transferListeners);
9090

91-
Aws::Client::AWSError<Aws::S3::S3Errors> FinalizeOnSuccess(
91+
OptionalError FinalizeOnSuccess(
9292
const std::shared_ptr<DownloadTransferState>& state) const override;
93-
void CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const override;
93+
OptionalError CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const override;
9494
const std::shared_ptr<DownloadDataReceiver>& GetDataReceiver() const override { return m_dataReceiver; }
9595

9696
private:

‎src/aws-cpp-sdk-s3-transfer/include/aws/s3-transfer/internal/UploadRequestImpl.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ class AWS_CORE_LOCAL UploadRequestImpl {
3636
UploadRequestImpl(UploadRequestImpl&&) = delete;
3737
UploadRequestImpl& operator=(UploadRequestImpl&&) = delete;
3838

39-
// Validates the source and seeds any state the transfer needs before dispatch.
40-
virtual Aws::Client::AWSError<Aws::S3::S3Errors> PrepareTransferState(
39+
// Validates the source and seeds any state the transfer needs before dispatch; empty on success.
40+
virtual OptionalError PrepareTransferState(
4141
const std::shared_ptr<UploadTransferState>& state) const = 0;
4242

4343
// Empty unless this request uploads from a file, in which case the CRT reads the file directly.
@@ -64,7 +64,7 @@ class AWS_CORE_LOCAL FileUploadImpl final : public UploadRequestImpl {
6464
Aws::String sourceFilePath,
6565
Aws::Vector<std::shared_ptr<UploadProgressListener>> transferListeners);
6666

67-
Aws::Client::AWSError<Aws::S3::S3Errors> PrepareTransferState(
67+
OptionalError PrepareTransferState(
6868
const std::shared_ptr<UploadTransferState>& state) const override;
6969
const Aws::String& GetSourceFilePath() const override { return m_sourceFilePath; }
7070

@@ -79,7 +79,7 @@ class AWS_CORE_LOCAL StreamUploadImpl final : public UploadRequestImpl {
7979
std::shared_ptr<Aws::IOStream> body,
8080
Aws::Vector<std::shared_ptr<UploadProgressListener>> transferListeners);
8181

82-
Aws::Client::AWSError<Aws::S3::S3Errors> PrepareTransferState(
82+
OptionalError PrepareTransferState(
8383
const std::shared_ptr<UploadTransferState>& state) const override;
8484
bool IsStreamUpload() const override { return true; }
8585
};

‎src/aws-cpp-sdk-s3-transfer/source/s3-transfer/DownloadRequest.cpp‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <aws/s3-transfer/internal/TransferState.h>
88
#include <aws/core/platform/FileSystem.h>
99
#include <aws/core/utils/UUID.h>
10+
#include <aws/core/utils/logging/LogMacros.h>
1011
#include <aws/core/utils/memory/AWSMemory.h>
1112

1213
#include <cassert>
@@ -56,33 +57,43 @@ FileDownloadImpl::FileDownloadImpl(
5657
Aws::String(Aws::Utils::UUID::RandomUUID()).substr(0, 8);
5758
}
5859

59-
Aws::Client::AWSError<Aws::S3::S3Errors> FileDownloadImpl::Validate() const {
60+
OptionalError FileDownloadImpl::Validate() const {
6061
if (m_destinationFilePath.empty()) {
6162
return Aws::Client::AWSError<Aws::S3::S3Errors>(
6263
Aws::S3::S3Errors::INVALID_PARAMETER_VALUE, "INVALID_PARAMETER_VALUE",
6364
"DownloadRequest destination file path must not be empty", false);
6465
}
65-
return Aws::Client::AWSError<Aws::S3::S3Errors>();
66+
return OptionalError();
6667
}
6768

68-
Aws::Client::AWSError<Aws::S3::S3Errors> FileDownloadImpl::FinalizeOnSuccess(
69+
OptionalError FileDownloadImpl::FinalizeOnSuccess(
6970
const std::shared_ptr<DownloadTransferState>&) const {
7071
// Windows MoveFileW won't overwrite; remove first (non-atomic).
7172
#ifdef _WIN32
7273
Aws::FileSystem::RemoveFileIfExists(m_destinationFilePath.c_str());
7374
#endif
7475
if (!Aws::FileSystem::RelocateFileOrDirectory(m_tempFilePath.c_str(), m_destinationFilePath.c_str())) {
75-
Aws::FileSystem::RemoveFileIfExists(m_tempFilePath.c_str());
76+
if (!Aws::FileSystem::RemoveFileIfExists(m_tempFilePath.c_str())) {
77+
AWS_LOGSTREAM_WARN(DOWNLOAD_REQUEST_ALLOCATION_TAG,
78+
"Could not remove the temp file after a failed rename; it remains at "
79+
<< m_tempFilePath);
80+
}
7681
return Aws::Client::AWSError<Aws::S3::S3Errors>(
7782
Aws::S3::S3Errors::UNKNOWN, "FileRenameFailure",
7883
"Downloaded data could not be moved to the destination path.", false);
7984
}
80-
return Aws::Client::AWSError<Aws::S3::S3Errors>();
85+
return OptionalError();
8186
}
8287

83-
void FileDownloadImpl::CleanupOnFailure(const std::shared_ptr<DownloadTransferState>&) const {
84-
// aws-c-s3 leaves recv_filepath in place on failure.
85-
Aws::FileSystem::RemoveFileIfExists(m_tempFilePath.c_str());
88+
OptionalError FileDownloadImpl::CleanupOnFailure(const std::shared_ptr<DownloadTransferState>&) const {
89+
// aws-c-s3 leaves recv_filepath in place on failure. If it cannot be removed, tell the caller so
90+
// the customer knows a partial file remains at the temp path.
91+
if (!Aws::FileSystem::RemoveFileIfExists(m_tempFilePath.c_str())) {
92+
return Aws::Client::AWSError<Aws::S3::S3Errors>(
93+
Aws::S3::S3Errors::UNKNOWN, "TempFileCleanupFailure",
94+
"Could not remove the temporary download file left at " + m_tempFilePath, false);
95+
}
96+
return OptionalError();
8697
}
8798

8899
StreamDownloadImpl::StreamDownloadImpl(
@@ -93,12 +104,14 @@ StreamDownloadImpl::StreamDownloadImpl(
93104
assert(m_dataReceiver && "DownloadRequest data receiver must not be null");
94105
}
95106

96-
Aws::Client::AWSError<Aws::S3::S3Errors> StreamDownloadImpl::FinalizeOnSuccess(
107+
OptionalError StreamDownloadImpl::FinalizeOnSuccess(
97108
const std::shared_ptr<DownloadTransferState>&) const {
98-
return Aws::Client::AWSError<Aws::S3::S3Errors>();
109+
return OptionalError();
99110
}
100111

101-
void StreamDownloadImpl::CleanupOnFailure(const std::shared_ptr<DownloadTransferState>&) const {}
112+
OptionalError StreamDownloadImpl::CleanupOnFailure(const std::shared_ptr<DownloadTransferState>&) const {
113+
return OptionalError();
114+
}
102115

103116
} // namespace Internal
104117

@@ -195,15 +208,15 @@ const Aws::S3::Model::GetObjectRequest& DownloadRequest::GetS3Request() const {
195208
return m_impl->GetS3Request();
196209
}
197210

198-
Aws::Client::AWSError<Aws::S3::S3Errors> DownloadRequest::Validate() const { return m_impl->Validate(); }
211+
Internal::OptionalError DownloadRequest::Validate() const { return m_impl->Validate(); }
199212

200-
Aws::Client::AWSError<Aws::S3::S3Errors> DownloadRequest::FinalizeOnSuccess(
213+
Internal::OptionalError DownloadRequest::FinalizeOnSuccess(
201214
const std::shared_ptr<DownloadTransferState>& state) const {
202215
return m_impl->FinalizeOnSuccess(state);
203216
}
204217

205-
void DownloadRequest::CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const {
206-
m_impl->CleanupOnFailure(state);
218+
Internal::OptionalError DownloadRequest::CleanupOnFailure(const std::shared_ptr<DownloadTransferState>& state) const {
219+
return m_impl->CleanupOnFailure(state);
207220
}
208221

209222
} // namespace Transfer

‎src/aws-cpp-sdk-s3-transfer/source/s3-transfer/UploadRequest.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ FileUploadImpl::FileUploadImpl(Aws::String bucket,
4343
assert(!m_sourceFilePath.empty() && "UploadRequest file source path must not be empty");
4444
}
4545

46-
Aws::Client::AWSError<Aws::S3::S3Errors> FileUploadImpl::PrepareTransferState(
46+
OptionalError FileUploadImpl::PrepareTransferState(
4747
const std::shared_ptr<UploadTransferState>&) const {
48-
return Aws::Client::AWSError<Aws::S3::S3Errors>();
48+
return OptionalError();
4949
}
5050

5151
StreamUploadImpl::StreamUploadImpl(Aws::String bucket,
@@ -57,7 +57,7 @@ StreamUploadImpl::StreamUploadImpl(Aws::String bucket,
5757
m_s3Request.SetBody(std::move(body));
5858
}
5959

60-
Aws::Client::AWSError<Aws::S3::S3Errors> StreamUploadImpl::PrepareTransferState(
60+
OptionalError StreamUploadImpl::PrepareTransferState(
6161
const std::shared_ptr<UploadTransferState>& state) const {
6262
const auto& body = m_s3Request.GetBody();
6363
if (!body) {
@@ -75,7 +75,7 @@ Aws::Client::AWSError<Aws::S3::S3Errors> StreamUploadImpl::PrepareTransferState(
7575
if (m_declaredLength) {
7676
state->totalBytes = m_declaredLength;
7777
}
78-
return Aws::Client::AWSError<Aws::S3::S3Errors>();
78+
return OptionalError();
7979
}
8080

8181
} // namespace Internal
@@ -210,7 +210,7 @@ const Aws::S3::Model::PutObjectRequest& UploadRequest::GetS3Request() const {
210210
return m_impl->GetS3Request();
211211
}
212212

213-
Aws::Client::AWSError<Aws::S3::S3Errors> UploadRequest::PrepareTransferState(
213+
Internal::OptionalError UploadRequest::PrepareTransferState(
214214
const std::shared_ptr<UploadTransferState>& state) const {
215215
return m_impl->PrepareTransferState(state);
216216
}

‎src/aws-cpp-sdk-s3-transfer/source/s3-transfer/internal/CrtOperations.cpp‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,10 @@ Aws::Client::CoreErrors MapCrtErrorCode(Aws::Crt::S3::S3ErrorCode crtErrorCode)
8888
}
8989
}
9090

91-
// SEP checksum validation drops error bodies on non-2xx GETs; recover RequestId from headers.
92-
Aws::String ExtractHeader(const Aws::Crt::Vector<Aws::Crt::Http::HttpHeader>& headers, const char* name) {
91+
// SEP checksum validation drops error bodies on non-2xx GETs; recover RequestId from headers. Returns
92+
// nothing when the header is absent, which is distinct from a header S3 sent with an empty value.
93+
Aws::Crt::Optional<Aws::String> ExtractHeader(const Aws::Crt::Vector<Aws::Crt::Http::HttpHeader>& headers,
94+
const char* name) {
9395
for (const auto& header : headers) {
9496
const Aws::String key = Aws::Utils::StringUtils::FromByteCursor(header.name);
9597
if (Aws::Utils::StringUtils::CaselessCompare(key.c_str(), name)) {
@@ -139,7 +141,11 @@ Aws::Client::AWSError<Aws::S3::S3Errors> MapCrtError(const Aws::Crt::S3::S3MetaR
139141
/*isRetryable*/ false);
140142
}
141143
error.SetResponseCode(static_cast<Aws::Http::HttpResponseCode>(result.responseStatus));
142-
error.SetRequestId(ExtractHeader(result.errorResponseHeaders, "x-amz-request-id"));
144+
// Only overwrite the request ID when S3 sent the header; an absent one must not blank it.
145+
const Aws::Crt::Optional<Aws::String> requestId = ExtractHeader(result.errorResponseHeaders, "x-amz-request-id");
146+
if (requestId) {
147+
error.SetRequestId(requestId.value());
148+
}
143149
error.SetResponseHeaders(ToHeaderCollection(result.errorResponseHeaders));
144150
return error;
145151
}
@@ -423,9 +429,9 @@ UploadHandle CrtOperations::DispatchUpload(S3TransferManagerImpl& impl, const Up
423429
}
424430

425431
{
426-
Aws::Client::AWSError<Aws::S3::S3Errors> prepareError = state->request.PrepareTransferState(state);
427-
if (!prepareError.GetExceptionName().empty()) {
428-
NotifyEarlyUploadFailure(state, std::move(prepareError));
432+
Internal::OptionalError prepareError = state->request.PrepareTransferState(state);
433+
if (prepareError) {
434+
NotifyEarlyUploadFailure(state, prepareError.value());
429435
return UploadHandle(std::move(handleImpl));
430436
}
431437
}
@@ -564,9 +570,9 @@ DownloadHandle CrtOperations::DispatchDownload(S3TransferManagerImpl& impl, cons
564570
}
565571

566572
{
567-
Aws::Client::AWSError<Aws::S3::S3Errors> validationError = state->request.Validate();
568-
if (!validationError.GetExceptionName().empty()) {
569-
NotifyEarlyDownloadFailure(state, std::move(validationError));
573+
Internal::OptionalError validationError = state->request.Validate();
574+
if (validationError) {
575+
NotifyEarlyDownloadFailure(state, validationError.value());
570576
return DownloadHandle(std::move(handleImpl));
571577
}
572578
}
@@ -648,11 +654,11 @@ DownloadHandle CrtOperations::DispatchDownload(S3TransferManagerImpl& impl, cons
648654

649655
options->SetFinishCallback([state](const Aws::Crt::S3::S3MetaRequestResult& result) {
650656
if (result.GetErrorCode() == Aws::Crt::S3::S3ErrorCode::Success) {
651-
Aws::Client::AWSError<Aws::S3::S3Errors> finalizeError = state->request.FinalizeOnSuccess(state);
652-
if (!finalizeError.GetExceptionName().empty()) {
657+
Internal::OptionalError finalizeError = state->request.FinalizeOnSuccess(state);
658+
if (finalizeError) {
653659
NotifyListeners(state, &DownloadProgressListener::OnTransferFailed,
654660
MakeDownloadSnapshot(state, state->transferredBytes.load()));
655-
state->promise.set_value(DownloadOutcome(std::move(finalizeError)));
661+
state->promise.set_value(DownloadOutcome(finalizeError.value()));
656662
return;
657663
}
658664

@@ -667,8 +673,13 @@ DownloadHandle CrtOperations::DispatchDownload(S3TransferManagerImpl& impl, cons
667673
Aws::MakeShared<DownloadResponse>(CRT_OPERATIONS_LOG_TAG, MakeDownloadResponse(state))));
668674
state->promise.set_value(DownloadOutcome(MakeDownloadResponse(state)));
669675
} else {
670-
state->request.CleanupOnFailure(state);
671676
auto error = MapCrtError(result);
677+
// The download already failed; if wiping the temp file also failed, fold that into the message
678+
// so the customer learns a partial file was left behind rather than losing it silently.
679+
Internal::OptionalError cleanupError = state->request.CleanupOnFailure(state);
680+
if (cleanupError) {
681+
error.SetMessage(error.GetMessage() + " (" + cleanupError.value().GetMessage() + ")");
682+
}
672683
NotifyListeners(state, &DownloadProgressListener::OnTransferFailed,
673684
MakeDownloadSnapshot(state, state->transferredBytes.load()));
674685
state->promise.set_value(DownloadOutcome(std::move(error)));

0 commit comments

Comments
 (0)