Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions generator/.DevConfigs/433a9a6d-b8ea-4676-b763-70711e8288e2.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"services": [
{
"serviceName": "S3",
"type": "minor",
"changeLogMessages": [
"Added progress tracking events to simple upload"
]
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,18 @@ internal partial class SimpleUploadCommand : BaseCommand
IAmazonS3 _s3Client;
TransferUtilityConfig _config;
TransferUtilityUploadRequest _fileTransporterRequest;
long _totalTransferredBytes;
private readonly long _contentLength;

internal SimpleUploadCommand(IAmazonS3 s3Client, TransferUtilityConfig config, TransferUtilityUploadRequest fileTransporterRequest)
{
this._s3Client = s3Client;
this._config = config;
this._fileTransporterRequest = fileTransporterRequest;

// Cache content length immediately while stream is accessible to avoid ObjectDisposedException in failure scenarios
this._contentLength = this._fileTransporterRequest.ContentLength;

var fileName = fileTransporterRequest.FilePath;
}

Expand Down Expand Up @@ -103,9 +109,48 @@ private PutObjectRequest ConstructRequest()

private void PutObjectProgressEventCallback(object sender, UploadProgressArgs e)
{
var progressArgs = new UploadProgressArgs(e.IncrementTransferred, e.TransferredBytes, e.TotalBytes,
e.CompensationForRetry, _fileTransporterRequest.FilePath);
// Keep track of the total transferred bytes so that we can also return this value in case of failure
long transferredBytes = Interlocked.Add(ref _totalTransferredBytes, e.IncrementTransferred - e.CompensationForRetry);
Comment on lines +112 to +113
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

The use of Interlocked.Add for thread-safe tracking of transferred bytes is correct, but consider potential race conditions where the final transferred bytes count might not accurately reflect the state at the exact moment of failure in multi-threaded scenarios.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@GarrettBeatty GarrettBeatty Oct 17, 2025

Choose a reason for hiding this comment

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

not sure if this really matters because this is supposed to be the current moment in time. also multi part upload has been doing this same logic

long transferredBytes = Interlocked.Add(ref _totalTransferredBytes, e.IncrementTransferred - e.CompensationForRetry);


var progressArgs = new UploadProgressArgs(e.IncrementTransferred, transferredBytes, _contentLength,
e.CompensationForRetry, _fileTransporterRequest.FilePath, _fileTransporterRequest);
this._fileTransporterRequest.OnRaiseProgressEvent(progressArgs);
}

private void FireTransferInitiatedEvent()
{
var initiatedArgs = new UploadInitiatedEventArgs(
request: _fileTransporterRequest,
filePath: _fileTransporterRequest.FilePath,
totalBytes: _contentLength
);

_fileTransporterRequest.OnRaiseTransferInitiatedEvent(initiatedArgs);
}

private void FireTransferCompletedEvent(TransferUtilityUploadResponse response)
{
var completedArgs = new UploadCompletedEventArgs(
request: _fileTransporterRequest,
response: response,
filePath: _fileTransporterRequest.FilePath,
transferredBytes: Interlocked.Read(ref _totalTransferredBytes),
totalBytes: _contentLength
);

_fileTransporterRequest.OnRaiseTransferCompletedEvent(completedArgs);
}

private void FireTransferFailedEvent()
{
var failedArgs = new UploadFailedEventArgs(
request: _fileTransporterRequest,
filePath: _fileTransporterRequest.FilePath,
transferredBytes: Interlocked.Read(ref _totalTransferredBytes),
totalBytes: _contentLength
);

_fileTransporterRequest.OnRaiseTransferFailedEvent(failedArgs);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,20 @@ await this.AsyncThrottler.WaitAsync(cancellationToken)
.ConfigureAwait(continueOnCapturedContext: false);
}

FireTransferInitiatedEvent();

var putRequest = ConstructRequest();
Comment on lines +41 to 43
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

[nitpick] Firing the initiated event before constructing the PutObjectRequest makes it harder to provide accurate metadata (like total bytes) if the file size or request normalization occurs during construction. Consider constructing or at least computing total size first, then firing the initiated event so the event carries accurate totals.

Suggested change
FireTransferInitiatedEvent();
var putRequest = ConstructRequest();
var putRequest = ConstructRequest();
FireTransferInitiatedEvent();

Copilot uses AI. Check for mistakes.
await _s3Client.PutObjectAsync(putRequest, cancellationToken)
var response = await _s3Client.PutObjectAsync(putRequest, cancellationToken)
.ConfigureAwait(continueOnCapturedContext: false);

var mappedResponse = ResponseMapper.MapPutObjectResponse(response);

FireTransferCompletedEvent(mappedResponse);
}
catch (Exception)
{
FireTransferFailedEvent();
throw;
}
finally
{
Expand Down
Loading