Skip to content

Commit 034a2f7

Browse files
amnguyeamnguye
authored
[Storage] [DataMovement] Change TransferOperation to IDisposable; Remove unnecessary Disposable inheritance on other internal classes (Azure#51637)
* Fix to remove completed transfer from transfers dictionary in transfer manager * WIP - dispose CancellationTokenSource, changed TransferInternalState is Dispose * WIP - chunkhandler to not disposable; channel processor to not disposable * WIP * Remove IDisposable off of chunk handler and job parts, processors; Export API * Fix tests * Cleanup * WIP * Dispose CTS after transfer cancels; Update tests * Update changelog * Update tests to reflect double pause causing exceptions * WIP - Removed unnecessary IDisposable type on TransferOperation, removed CTS from ChannelProcessor * WIP - update rest of tests to reflect removal of CT needed from ChannelProcessor * WIP - test fixes * WIP * WIP - Transfer Progress Tracker undo unnecessary changes * Export API; update chunk handler tests to trycomplete * Clear _transfers at TransferManager disposes * Update to more accurate Changelog * Dispose CTS once transfer completes/pauses * PR Comments addressed - Reverted RemoveTransferAsync delegate back to TransferManager passed; moved clear transfers to end of dispose; spacing * Add back CancellationToken to QueueAsync and delegates; Update tests to reflect changes * PR feedback - lint error with having unnecessary else; fix tabbing in tests * Refactored TryComplete to not return the bool and renamed to CleanupAsync * Fix spacing * Attempt to fix ProcessJobToJobPartAsync tests * Fix issue with cancellation propogating OperationCancelled, when transfer is already cancelling * PR Feedback - removed disposing CTS upon cancellation; removed unncessary imports; removed default CT;
1 parent 5254617 commit 034a2f7

40 files changed

+600
-504
lines changed

‎sdk/storage/Azure.Storage.Common/src/stress/Shared/Metrics.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,12 +87,12 @@ public class Metrics
8787
/// Initializes a new instance of the <see cref="Metrics" /> class.
8888
/// </summary>
8989
///
90-
/// <param name="instrumentationKey">The instrumentation key associated with the Application Insights resource.</param>
90+
/// <param name="connectionString">The instrumentation key associated with the Application Insights resource.</param>
9191
///
92-
public Metrics(string instrumentationKey)
92+
public Metrics(string connectionString)
9393
{
9494
var configuration = TelemetryConfiguration.CreateDefault();
95-
configuration.InstrumentationKey = instrumentationKey;
95+
configuration.ConnectionString = connectionString;
9696

9797
Client = new TelemetryClient(configuration);
9898
}

‎sdk/storage/Azure.Storage.Common/tests/Shared/TestHelper.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,11 +164,11 @@ public static void AssertInconclusiveRecordingFriendly(RecordedTestMode mode, st
164164
}
165165
}
166166

167-
public static CancellationToken GetTimeoutToken(int seconds)
167+
public static CancellationTokenSource GetTimeoutTokenSource(int seconds)
168168
{
169169
CancellationTokenSource cts = new();
170170
cts.CancelAfter(TimeSpan.FromSeconds(seconds));
171-
return cts.Token;
171+
return cts;
172172
}
173173
}
174174
}

‎sdk/storage/Azure.Storage.DataMovement.Blobs/CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@
77
### Breaking Changes
88

99
### Bugs Fixed
10+
- Resolved memory leak issue with `CancellationTokenSource` usage not being properly disposed, namely in the following areas:
11+
- `TransferOperation` disposes the `CancellationTokenSource` after transfer reaches a `Completed` or `Paused` state
12+
- `TransferManager` uses a `CancellationTokenSource` also does not link the`CancellationToken` passed to it's methods
13+
- Removed usage of `CancellationTokenSource` from handling the chunking of large transfers. This only affects transfers that cannot be completed in one request.
14+
- Fixed bug where cached referenced `TransferOperation`s from the `TransferManager` were not being cleared on dispose.
15+
- Fixed bug where referenced `TransferOperation` from the transfers stored in the `TransferManager` after they reach a `Completed` or `Paused` state where not being removed.
1016

1117
### Other Changes
1218

‎sdk/storage/Azure.Storage.DataMovement.Blobs/tests/BlobContainerClientExtensionsIntegrationTests.cs‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ public async Task StartUploadDirectoryAsync_WithOptions()
116116

117117
// Act
118118
TransferOperation transferOperation = await CreateStartUploadDirectoryAsync_WithOptions(directoryPath, containerClient, false, options, 1);
119-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
119+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
120120
await TestTransferWithTimeout.WaitForCompletionAsync(
121121
transferOperation,
122122
testEventsRaised,
@@ -157,7 +157,7 @@ public async Task StartUploadDirectoryAsync_WithDirectoryPrefix()
157157

158158
// Act
159159
TransferOperation transferOperation = await CreateStartUploadDirectoryAsync_WithDirectoryPrefix(directoryPath, containerClient, blobDirectoryPrefix, 1);
160-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
160+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
161161
await transferOperation.WaitForCompletionAsync(cancellationTokenSource.Token).ConfigureAwait(false);
162162

163163
// Assert
@@ -197,7 +197,7 @@ public async Task StartUploadDirectoryAsync_WithOptions_Failed()
197197

198198
// Act
199199
TransferOperation transferOperation = await CreateStartUploadDirectoryAsync_WithOptions(directoryPath, containerClient, true, options, 1);
200-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
200+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
201201
await TestTransferWithTimeout.WaitForCompletionAsync(
202202
transferOperation,
203203
testEventsRaised,
@@ -231,7 +231,7 @@ public async Task StartUploadDirectoryAsync_WithOptions_Skipped()
231231

232232
// Act
233233
TransferOperation transferOperation = await CreateStartUploadDirectoryAsync_WithOptions(directoryPath, containerClient, true, options, 1);
234-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
234+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
235235
await TestTransferWithTimeout.WaitForCompletionAsync(
236236
transferOperation,
237237
testEventsRaised,
@@ -349,7 +349,7 @@ public async Task StartDownloadToDirectoryAsync_WithOptions()
349349

350350
// Act
351351
TransferOperation transferOperation = await CreateStartDownloadToDirectoryAsync_WithOptions(directoryPath, containerClient, options, 1);
352-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
352+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
353353
await TestTransferWithTimeout.WaitForCompletionAsync(
354354
transferOperation,
355355
testEventsRaised,
@@ -389,7 +389,7 @@ public async Task StartDownloadToDirectoryAsync_WithDirectoryPrefix()
389389

390390
// Act
391391
TransferOperation transferOperation = await CreateStartDownloadToDirectoryAsync_WithDirectoryPrefix(directoryPath, containerClient, prefixFilter, 1);
392-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
392+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
393393
await transferOperation.WaitForCompletionAsync(cancellationTokenSource.Token).ConfigureAwait(false);
394394

395395
// Assert
@@ -423,7 +423,7 @@ public async Task StartDownloadToDirectoryAsync_WithOptions_Failed()
423423

424424
// Act
425425
TransferOperation transferOperation = await CreateStartDownloadToDirectoryAsync_WithOptions(directoryPath, containerClient, options, 1);
426-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
426+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
427427
await TestTransferWithTimeout.WaitForCompletionAsync(
428428
transferOperation,
429429
testEventsRaised,
@@ -434,8 +434,8 @@ await TestTransferWithTimeout.WaitForCompletionAsync(
434434
Assert.IsTrue(transferOperation.HasCompleted);
435435
Assert.AreEqual(TransferState.Completed, transferOperation.Status.State);
436436
Assert.AreEqual(true, transferOperation.Status.HasFailedItems);
437-
Assert.IsTrue(testEventsRaised.FailedEvents.First().Exception.Message.Contains("Cannot overwrite file."));
438437
await testEventsRaised.AssertContainerCompletedWithFailedCheck(1);
438+
Assert.IsTrue(testEventsRaised.FailedEvents.First().Exception.Message.Contains("Cannot overwrite file."));
439439
}
440440

441441
[RecordedTest]
@@ -462,7 +462,7 @@ public async Task StartDownloadToDirectoryAsync_WithOptions_Skipped()
462462

463463
// Act
464464
TransferOperation transferOperation = await CreateStartDownloadToDirectoryAsync_WithOptions(directoryPath, containerClient, options, 1);
465-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
465+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
466466
await TestTransferWithTimeout.WaitForCompletionAsync(
467467
transferOperation,
468468
testEventsRaised,

‎sdk/storage/Azure.Storage.DataMovement.Blobs/tests/BlobContainerServiceToServiceJobTests.cs‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,13 @@ await checkpointer.AddNewJobAsync(
110110
It.IsAny<StorageResourceContainer>(),
111111
It.IsAny<CancellationToken>()))
112112
.Returns(GetStorageResourceItemsAsyncEnumerable(blobItems));
113+
await using TransferManager manager = new TransferManager();
114+
TransferOperation transferOperation = new TransferOperation(id: transferId)
115+
{
116+
TransferManager = manager
117+
};
113118
TransferJobInternal transferJob = new(
114-
new TransferOperation(id: transferId),
119+
transferOperation,
115120
sourceMock.Object,
116121
destinationMock.Object,
117122
ServiceToServiceJobPart.CreateJobPartAsync,
@@ -165,8 +170,13 @@ await checkpointer.AddNewJobAsync(
165170
It.IsAny<StorageResourceContainer>(),
166171
It.IsAny<CancellationToken>()))
167172
.Returns(GetStorageResourceItemsAsyncEnumerable(blobItems));
173+
await using TransferManager manager = new TransferManager();
174+
TransferOperation transferOperation = new TransferOperation(id: transferId)
175+
{
176+
TransferManager = manager
177+
};
168178
TransferJobInternal transferJob = new(
169-
new TransferOperation(id: transferId),
179+
transferOperation,
170180
sourceMock.Object,
171181
destinationMock.Object,
172182
ServiceToServiceJobPart.CreateJobPartAsync,

‎sdk/storage/Azure.Storage.DataMovement.Blobs/tests/PageBlobStartTransferCopyTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ public async Task CopyPremiumPageBlob_AccessTier(TransferPropertiesTestType prop
363363
sourceResource,
364364
destinationResource,
365365
options);
366-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
366+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
367367
await TestTransferWithTimeout.WaitForCompletionAsync(
368368
transfer,
369369
testEventsRaised,

‎sdk/storage/Azure.Storage.DataMovement.Blobs/tests/ProgressHandlerTests.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ private async Task TransferAndAssertProgress(
139139
transferOptions.CreationMode = createMode;
140140

141141
TransferOperation transfer = await transferManager.StartTransferAsync(source, destination, transferOptions);
142-
CancellationTokenSource tokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(waitTime));
142+
using CancellationTokenSource tokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(waitTime));
143143
await transfer.WaitForCompletionAsync(tokenSource.Token);
144144

145145
ProgressHandlerAsserts.AssertFileProgress(progressHandler.Updates, fileCount, skippedCount, failedCount);
@@ -346,7 +346,7 @@ await blobProvider.FromContainerAsync(source.Container.Uri),
346346
await Task.Delay(delayInMs);
347347

348348
// Pause transfer
349-
CancellationTokenSource tokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
349+
using CancellationTokenSource tokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
350350
await transferManager.PauseTransferAsync(transfer.Id, tokenSource.Token);
351351
Assert.AreEqual(TransferState.Paused, transfer.Status.State);
352352

@@ -358,8 +358,8 @@ await blobProvider.FromContainerAsync(source.Container.Uri),
358358
transfer.Id,
359359
transferOptions);
360360

361-
tokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
362-
await resumeTransfer.WaitForCompletionAsync(tokenSource.Token);
361+
using CancellationTokenSource tokenSource2 = new CancellationTokenSource(TimeSpan.FromSeconds(30));
362+
await resumeTransfer.WaitForCompletionAsync(tokenSource2.Token);
363363

364364
// Assert
365365
Assert.AreEqual(TransferState.Completed, resumeTransfer.Status.State);

‎sdk/storage/Azure.Storage.DataMovement.Blobs/tests/StartTransferCheckpointerTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ public async Task CheckpointerWithSasAsync()
9090
transferOptions).ConfigureAwait(false);
9191

9292
// Act
93-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(20));
93+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(20));
9494
await transfer.WaitForCompletionAsync(cancellationTokenSource.Token).ConfigureAwait(false);
9595

9696
// Check the transfer files made and the source and destination

‎sdk/storage/Azure.Storage.DataMovement.Files.Shares/CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@
77
### Breaking Changes
88

99
### Bugs Fixed
10+
- Resolved memory leak issue with `CancellationTokenSource` usage not being properly disposed, namely in the following areas:
11+
- `TransferOperation` disposes the `CancellationTokenSource` after transfer reaches a `Completed` or `Paused` state
12+
- `TransferManager` uses a `CancellationTokenSource` also does not link the`CancellationToken` passed to it's methods
13+
- Removed usage of `CancellationTokenSource` from handling the chunking of large transfers. This only affects transfers that cannot be completed in one request.
14+
- Fixed bug where cached referenced `TransferOperation`s from the `TransferManager` were not being cleared on dispose.
15+
- Fixed bug where referenced `TransferOperation` from the transfers stored in the `TransferManager` after they reach a `Completed` or `Paused` state where not being removed.
1016

1117
### Other Changes
1218

‎sdk/storage/Azure.Storage.DataMovement.Files.Shares/tests/ShareDirectoryStartTransferCopyTests.cs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -801,7 +801,7 @@ public async Task ShareDirectoryToShareDirectory_PreserveSmb_NoExistingDestNoOve
801801
options).ConfigureAwait(false);
802802

803803
// Act
804-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
804+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
805805
await TestTransferWithTimeout.WaitForCompletionAsync(
806806
transfer,
807807
testEventsRaised,
@@ -1135,7 +1135,7 @@ public async Task ShareDirectoryToShareDirectory_PreserveNfs_NoExistingDestNoOve
11351135
options).ConfigureAwait(false);
11361136

11371137
// Act
1138-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
1138+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
11391139
await TestTransferWithTimeout.WaitForCompletionAsync(
11401140
transfer,
11411141
testEventsRaised,
@@ -1491,7 +1491,7 @@ public async Task ShareDirectoryToShareDirectory_SmbDestinationOptionsOverride()
14911491
destinationResource,
14921492
options).ConfigureAwait(false);
14931493

1494-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
1494+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
14951495
await TestTransferWithTimeout.WaitForCompletionAsync(
14961496
transfer,
14971497
testEventsRaised,
@@ -1584,7 +1584,7 @@ public async Task ShareDirectoryToShareDirectory_NfsDestinationOptionsOverride(b
15841584
destinationResource,
15851585
options).ConfigureAwait(false);
15861586

1587-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
1587+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
15881588
await TestTransferWithTimeout.WaitForCompletionAsync(
15891589
transfer,
15901590
testEventsRaised,
@@ -1684,7 +1684,7 @@ public async Task ValidateProtocolAsync_SmbShareDirectoryToSmbShareDirectory_Com
16841684
options).ConfigureAwait(false);
16851685

16861686
// Act
1687-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
1687+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
16881688
await TestTransferWithTimeout.WaitForCompletionAsync(
16891689
transfer,
16901690
testEventsRaised,
@@ -1772,7 +1772,7 @@ public async Task ValidateProtocolAsync_NfsShareDirectoryToNfsShareDirectory_Com
17721772
options).ConfigureAwait(false);
17731773

17741774
// Act
1775-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
1775+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
17761776
await TestTransferWithTimeout.WaitForCompletionAsync(
17771777
transfer,
17781778
testEventsRaised,
@@ -1857,7 +1857,7 @@ public async Task ShareDirectoryToShareDirectory_NfsHardLink()
18571857
options).ConfigureAwait(false);
18581858

18591859
// Act
1860-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
1860+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
18611861
await TestTransferWithTimeout.WaitForCompletionAsync(
18621862
transfer,
18631863
testEventsRaised,
@@ -1926,7 +1926,7 @@ public async Task ShareDirectoryToShareDirectory_NfsSymbolicLink()
19261926
options).ConfigureAwait(false);
19271927

19281928
// Act
1929-
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
1929+
using CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
19301930
await TestTransferWithTimeout.WaitForCompletionAsync(
19311931
transfer,
19321932
testEventsRaised,

0 commit comments

Comments
 (0)