Skip to content

Split out ForceCloseHandles into two methods #8134

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

Merged
JoshLove-msft merged 5 commits into from
Oct 18, 2019
Merged

Split out ForceCloseHandles into two methods #8134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b9116ec
Split out ForceCloseHandles into two methods
JoshLove-msft Oct 16, 2019
26835e1
Update return type and do looping ourselves for All API
JoshLove-msft Oct 16, 2019
58cd285
rebase
JoshLove-msft Oct 16, 2019
ba7e239
fix nits
JoshLove-msft Oct 17, 2019
22c1599
update test
JoshLove-msft Oct 17, 2019
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: 9 additions & 2 deletions sdk/storage/Azure.Storage.Common/src/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,11 @@ internal static class File

public const string SetHttpHeadersOperationName =
"Azure.Storage.Files.FileClient.SetHttpHeaders";
public const string ForceCloseAllHandlesOperationName =
"Azure.Storage.Files.FileClient.ForceCloseAllHandles";
public const string ForceCloseHandleOperationName =
"Azure.Storage.Files.FileClient.ForceCloseHandle";

internal static class Directory
{
public const string CreateOperationName =
Expand All @@ -309,8 +314,10 @@ internal static class Directory
"Azure.Storage.Files.DirectoryClient.ListFilesAndDirectoriesSegment";
public const string GetHandlesOperationName =
"Azure.Storage.Files.DirectoryClient.ListHandles";
public const string ForceCloseHandlesOperationName =
"Azure.Storage.Files.DirectoryClient.ForceCloseHandles";
public const string ForceCloseAllHandlesOperationName =
"Azure.Storage.Files.DirectoryClient.ForceCloseAllHandles";
public const string ForceCloseHandleOperationName =
"Azure.Storage.Files.DirectoryClient.ForceCloseHandle";
}

internal static class Service
Expand Down
201 changes: 152 additions & 49 deletions sdk/storage/Azure.Storage.Files/src/DirectoryClient.cs
View file
Open in desktop

Large diffs are not rendered by default.

179 changes: 135 additions & 44 deletions sdk/storage/Azure.Storage.Files/src/FileClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2387,9 +2387,8 @@ internal async Task<Response<StorageHandlesSegment>> GetHandlesInternal(

#region ForceCloseHandles
/// <summary>
/// The <see cref="ForceCloseHandles"/> operation closes a handle or handles opened on a file
/// at the service. It supports closing a single handle specified by <paramref name="handleId"/> or
/// or closing all handles opened on that resource.
/// The <see cref="ForceCloseHandle"/> operation closes a handle opened on a file
/// at the service. It supports closing a single handle specified by <paramref name="handleId"/>.
///
/// This API is intended to be used alongside <see cref="GetHandlesAsync"/> to force close handles that
/// block operations. These handles may have leaked or been lost track of by
Expand All @@ -2400,44 +2399,35 @@ internal async Task<Response<StorageHandlesSegment>> GetHandlesInternal(
/// For more information, see <see href="https://docs.microsoft.com/en-us/rest/api/storageservices/force-close-handles"/>.
/// </summary>
/// <param name="handleId">
/// Optional. Specifies the handle ID to be closed. If not specified, or if equal to &quot;*&quot;, will close all handles.
/// </param>
/// <param name="marker">
/// An optional string value that identifies the segment of the handles
/// to be closed with the next call to <see cref="ForceCloseHandles"/>. The
/// operation returns a non-empty <see cref="StorageClosedHandlesSegment.Marker"/>
/// if the operation did not return all items remaining to be
/// closed with the current segment. The NextMarker value can
/// be used as the value for the <paramref name="marker"/> parameter
/// in a subsequent call to request the closure of the next segment of handles.
/// Specifies the handle ID to be closed.
/// </param>
/// <param name="cancellationToken">
/// Optional <see cref="CancellationToken"/> to propagate
/// notifications that the operation should be cancelled.
/// </param>
/// <returns>
/// A <see cref="Response{StorageClosedHandlesSegment}"/> describing a
/// segment of the handles closed.
/// A <see cref="Response"/> describing the status of the
/// <see cref="ForceCloseHandle"/> operation.
/// </returns>
/// <remarks>
/// A <see cref="StorageRequestFailedException"/> will be thrown if
/// a failure occurs.
/// </remarks>
public virtual Response<StorageClosedHandlesSegment> ForceCloseHandles(
string handleId = Constants.CloseAllHandles,
string marker = default,
public virtual Response ForceCloseHandle(
string handleId,
CancellationToken cancellationToken = default) =>
ForceCloseHandlesInternal(
handleId,
marker,
null,
false, // async,
cancellationToken)
.EnsureCompleted();
cancellationToken,
Constants.File.ForceCloseHandleOperationName)
.EnsureCompleted()
.GetRawResponse();

/// <summary>
/// The <see cref="ForceCloseHandlesAsync"/> operation closes a handle or handles opened on a file
/// at the service. It supports closing a single handle specified by <paramref name="handleId"/> or
/// or closing all handles opened on that resource.
/// The <see cref="ForceCloseHandleAsync"/> operation closes a handle opened on a file
/// at the service. It supports closing a single handle specified by <paramref name="handleId"/>.
///
/// This API is intended to be used alongside <see cref="GetHandlesAsync"/> to force close handles that
/// block operations. These handles may have leaked or been lost track of by
Expand All @@ -2448,40 +2438,136 @@ public virtual Response<StorageClosedHandlesSegment> ForceCloseHandles(
/// For more information, see <see href="https://docs.microsoft.com/en-us/rest/api/storageservices/force-close-handles"/>.
/// </summary>
/// <param name="handleId">
/// Optional. Specifies the handle ID to be closed. If not specified, or if equal to &quot;*&quot;, will close all handles.
/// </param>
/// <param name="marker">
/// An optional string value that identifies the segment of the handles
/// to be closed with the next call to <see cref="ForceCloseHandlesAsync"/>. The
/// operation returns a non-empty <see cref="StorageClosedHandlesSegment.Marker"/>
/// if the operation did not return all items remaining to be
/// closed with the current segment. The NextMarker value can
/// be used as the value for the <paramref name="marker"/> parameter
/// in a subsequent call to request the closure of the next segment of handles.
/// Specifies the handle ID to be closed.
/// </param>
/// <param name="cancellationToken">
/// Optional <see cref="CancellationToken"/> to propagate
/// notifications that the operation should be cancelled.
/// </param>
/// <returns>
/// A <see cref="Response{StorageClosedHandlesSegment}"/> describing a
/// segment of the handles closed.
/// A <see cref="Response"/> describing the status of the
/// <see cref="ForceCloseHandle"/> operation.
/// </returns>
/// <remarks>
/// A <see cref="StorageRequestFailedException"/> will be thrown if
/// a failure occurs.
/// </remarks>
public virtual async Task<Response<StorageClosedHandlesSegment>> ForceCloseHandlesAsync(
string handleId = Constants.CloseAllHandles,
string marker = default,
public virtual async Task<Response> ForceCloseHandleAsync(
string handleId,
CancellationToken cancellationToken = default) =>
await ForceCloseHandlesInternal(
(await ForceCloseHandlesInternal(
handleId,
marker,
null,
true, // async,
cancellationToken,
Constants.File.ForceCloseHandleOperationName)
.ConfigureAwait(false)).
GetRawResponse();

/// <summary>
/// The <see cref="ForceCloseAllHandles"/> operation closes all handles opened on a file
/// at the service.
///
/// This API is intended to be used alongside <see cref="GetHandlesAsync"/> to force close handles that
/// block operations. These handles may have leaked or been lost track of by
/// SMB clients. The API has client-side impact on the handle being closed, including user visible
/// errors due to failed attempts to read or write files. This API is not intended for use as a replacement
/// or alternative for SMB close.
///
/// For more information, see <see href="https://docs.microsoft.com/en-us/rest/api/storageservices/force-close-handles"/>.
/// </summary>
/// <param name="cancellationToken">
/// Optional <see cref="CancellationToken"/> to propagate
/// notifications that the operation should be cancelled.
/// </param>
/// <returns>
/// The number of handles closed.
/// </returns>
/// <remarks>
/// A <see cref="StorageRequestFailedException"/> will be thrown if
/// a failure occurs.
/// </remarks>
public virtual int ForceCloseAllHandles(
CancellationToken cancellationToken = default) =>
ForceCloseAllHandlesInternal(
false, // async,
cancellationToken)
.EnsureCompleted();

/// <summary>
/// The <see cref="ForceCloseAllHandlesAsync"/> operation closes all handles opened on a file
/// at the service.
///
/// This API is intended to be used alongside <see cref="GetHandlesAsync"/> to force close handles that
/// block operations. These handles may have leaked or been lost track of by
/// SMB clients. The API has client-side impact on the handle being closed, including user visible
/// errors due to failed attempts to read or write files. This API is not intended for use as a replacement
/// or alternative for SMB close.
///
/// For more information, see <see href="https://docs.microsoft.com/en-us/rest/api/storageservices/force-close-handles"/>.
/// </summary>
/// <param name="cancellationToken">
/// Optional <see cref="CancellationToken"/> to propagate
/// notifications that the operation should be cancelled.
/// </param>
/// <returns>
/// The number of handles closed.
/// </returns>
/// <remarks>
/// A <see cref="StorageRequestFailedException"/> will be thrown if
/// a failure occurs.
/// </remarks>
public virtual async Task<int> ForceCloseAllHandlesAsync(
CancellationToken cancellationToken = default) =>
await ForceCloseAllHandlesInternal(
true, // async,
cancellationToken)
.ConfigureAwait(false);

/// <summary>
/// The <see cref="ForceCloseAllHandlesInternal"/> operation closes a handle or handles opened on a file
/// at the service. It supports closing all handles opened on that resource.
///
/// This API is intended to be used alongside <see cref="GetHandlesAsync"/> to force close handles that
/// block operations. These handles may have leaked or been lost track of by
/// SMB clients. The API has client-side impact on the handle being closed, including user visible
/// errors due to failed attempts to read or write files. This API is not intended for use as a replacement
/// or alternative for SMB close.
///
/// For more information, see <see href="https://docs.microsoft.com/en-us/rest/api/storageservices/force-close-handles"/>.
/// </summary>
/// <param name="async">
/// Whether to invoke the operation asynchronously.
/// </param>
/// <param name="cancellationToken">
/// Optional <see cref="CancellationToken"/> to propagate
/// notifications that the operation should be cancelled.
/// </param>
/// <returns>
/// The number of handles closed.
/// </returns>
/// <remarks>
/// A <see cref="StorageRequestFailedException"/> will be thrown if
/// a failure occurs.
/// </remarks>
private async Task<int> ForceCloseAllHandlesInternal(
bool async,
CancellationToken cancellationToken)
{
int handlesClosed = 0;
string marker = null;
do
{
Response<StorageClosedHandlesSegment> response =
await ForceCloseHandlesInternal(Constants.CloseAllHandles, marker, async, cancellationToken).ConfigureAwait(false);
marker = response.Value.Marker;
handlesClosed += response.Value.NumberOfHandlesClosed;

} while (!string.IsNullOrEmpty(marker));

return handlesClosed;
}

/// <summary>
/// The <see cref="ForceCloseHandlesInternal"/> operation closes a handle or handles opened on a file
/// at the service. It supports closing a single handle specified by <paramref name="handleId"/> or
Expand All @@ -2500,7 +2586,7 @@ await ForceCloseHandlesInternal(
/// </param>
/// <param name="marker">
/// An optional string value that identifies the segment of the handles
/// to be closed with the next call to <see cref="ForceCloseHandlesAsync"/>. The
/// to be closed with the next call to <see cref="ForceCloseAllHandlesAsync"/>. The
/// operation returns a non-empty <see cref="StorageClosedHandlesSegment.Marker"/>
/// if the operation did not return all items remaining to be
/// closed with the current segment. The NextMarker value can
Expand All @@ -2514,6 +2600,9 @@ await ForceCloseHandlesInternal(
/// Optional <see cref="CancellationToken"/> to propagate
/// notifications that the operation should be cancelled.
/// </param>
/// <param name="operationName">
/// Optional. Used to indicate the name of the operation.
/// </param>
/// <returns>
/// A <see cref="Response{StorageClosedHandlesSegment}"/> describing a
/// segment of the handles closed.
Expand All @@ -2526,7 +2615,8 @@ private async Task<Response<StorageClosedHandlesSegment>> ForceCloseHandlesInter
string handleId,
string marker,
bool async,
CancellationToken cancellationToken)
CancellationToken cancellationToken,
string operationName = Constants.File.ForceCloseAllHandlesOperationName)
{
using (Pipeline.BeginLoggingScope(nameof(FileClient)))
{
Expand All @@ -2545,7 +2635,8 @@ private async Task<Response<StorageClosedHandlesSegment>> ForceCloseHandlesInter
marker: marker,
Copy link
Copy Markdown
Member

@tg-msft tg-msft Oct 16, 2019

Choose a reason for hiding this comment

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

This needs to change. We're going to wrap this in a loop that continues executing until no additional marker is returned or it throws.

handleId: handleId,
async: async,
cancellationToken: cancellationToken)
cancellationToken: cancellationToken,
operationName: operationName)
.ConfigureAwait(false);
}
catch (Exception ex)
Expand Down
25 changes: 20 additions & 5 deletions sdk/storage/Azure.Storage.Files/tests/DirectoryClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,10 +535,10 @@ public async Task ForceCloseHandles_Min()
using (GetNewDirectory(out DirectoryClient directory))
{
// Act
Response<StorageClosedHandlesSegment> response = await directory.ForceCloseHandlesAsync();
int handlesClosed = await directory.ForceCloseAllHandlesAsync();

// Assert
Assert.AreEqual(0, response.Value.NumberOfHandlesClosed);
Assert.AreEqual(0, handlesClosed);

}
}
Expand All @@ -550,10 +550,10 @@ public async Task ForceCloseHandles_Recursive()
using (GetNewDirectory(out DirectoryClient directory))
{
// Act
Response<StorageClosedHandlesSegment> response = await directory.ForceCloseHandlesAsync(recursive: true);
int handlesClosed = await directory.ForceCloseAllHandlesAsync(recursive: true);

// Assert
Assert.AreEqual(0, response.Value.NumberOfHandlesClosed);
Assert.AreEqual(0, handlesClosed);

}
}
Expand All @@ -568,12 +568,27 @@ public async Task ForceCloseHandles_Error()

// Act
await TestHelper.AssertExpectedExceptionAsync<StorageRequestFailedException>(
directory.ForceCloseHandlesAsync(),
directory.ForceCloseAllHandlesAsync(),
actualException => Assert.AreEqual("ResourceNotFound", actualException.ErrorCode));

}
}

[Test]
public async Task ForceCloseHandle_Error()
{
// Arrange
using (GetNewShare(out ShareClient share))
{
DirectoryClient directory = InstrumentClient(share.GetDirectoryClient(GetNewDirectoryName()));
AsyncPageable<StorageFileHandle> handles = directory.GetHandlesAsync();
// Act
await TestHelper.AssertExpectedExceptionAsync<StorageRequestFailedException>(
directory.ForceCloseHandleAsync("nonExistantHandleId"),
actualException => Assert.AreEqual("InvalidHeaderValue", actualException.ErrorCode));
}
}

[Test]
public async Task CreateSubdirectoryAsync()
{
Expand Down
22 changes: 19 additions & 3 deletions sdk/storage/Azure.Storage.Files/tests/FileClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,10 +1157,10 @@ public async Task ForceCloseHandles_Min()
using (GetNewFile(out FileClient file))
{
// Act
Response<StorageClosedHandlesSegment> response = await file.ForceCloseHandlesAsync();
int handlesClosed = await file.ForceCloseAllHandlesAsync();

// Assert
Assert.AreEqual(0, response.Value.NumberOfHandlesClosed);
Assert.AreEqual(0, handlesClosed);
}
}

Expand All @@ -1174,12 +1174,28 @@ public async Task ForceCloseHandles_Error()

// Act
await TestHelper.AssertExpectedExceptionAsync<StorageRequestFailedException>(
file.ForceCloseHandlesAsync(),
file.ForceCloseAllHandlesAsync(),
actualException => Assert.AreEqual("ResourceNotFound", actualException.ErrorCode));

}
}

[Test]
public async Task ForceCloseHandle_Error()
{
// Arrange
using (GetNewDirectory(out DirectoryClient directory))
{
FileClient file = InstrumentClient(directory.GetFileClient(GetNewDirectoryName()));

// Act
await TestHelper.AssertExpectedExceptionAsync<StorageRequestFailedException>(
file.ForceCloseHandleAsync("nonExistantHandleId"),
actualException => Assert.AreEqual("InvalidHeaderValue", actualException.ErrorCode));

}
}

private async Task WaitForCopy(FileClient file, int milliWait = 200)
{
CopyStatus status = CopyStatus.Pending;
Expand Down
Loading