Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Release pinned buffers correctly for WinHttpHandler, WinHttpWebSocket #6500

Merged
merged 1 commit into from
Feb 29, 2016
Merged

Release pinned buffers correctly for WinHttpHandler, WinHttpWebSocket #6500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 27 additions & 1 deletion src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public static WinHttpRequestState FromIntPtr(IntPtr gcHandle)
{
GCHandle stateHandle = GCHandle.FromIntPtr(gcHandle);
return (WinHttpRequestState)stateHandle.Target;
}
}

public IntPtr ToIntPtr()
{
Expand Down Expand Up @@ -123,6 +123,22 @@ public Func<
public long? ExpectedBytesToRead { get; set; }
public long CurrentBytesRead { get; set; }

// TODO (Issue 2505): temporary pinned buffer caches of 1 item. Will be replaced by PinnableBufferCache.
private GCHandle _cachedReceivePinnedBuffer = default(GCHandle);

public void PinReceiveBuffer(byte[] buffer)
{
if (!_cachedReceivePinnedBuffer.IsAllocated || _cachedReceivePinnedBuffer.Target != buffer)
Copy link
Member

Choose a reason for hiding this comment

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

What guarantees do we have that there isn't still an outstanding operation using the old target? Maybe there's something we can assert, about there not being any outstanding operations or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The WinHttpRequestState object relies on the caller (and there is only one caller) to assert that. In particular, the only async operation possible on this pinned receive buffer is from WinHttpResponseStream.ReadAsync(). There is already a check in that public API surface to reject calls to it if the previous ReadAsync is in progress.

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW #2501 will ensure more consistency checks as well.

{
if (_cachedReceivePinnedBuffer.IsAllocated)
{
_cachedReceivePinnedBuffer.Free();
}

_cachedReceivePinnedBuffer = GCHandle.Alloc(buffer, GCHandleType.Pinned);
}
}

#region IDisposable Members
private void Dispose(bool disposing)
{
Expand All @@ -147,7 +163,17 @@ private void Dispose(bool disposing)

if (_operationHandle.IsAllocated)
{
// This method only gets called when the WinHTTP request handle is fully closed and thus all
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything we can assert to ensure that?

// async operations are done. So, it is safe at this point to unpin the buffers and release
// the strong GCHandle for this object.
if (_cachedReceivePinnedBuffer.IsAllocated)
{
_cachedReceivePinnedBuffer.Free();
_cachedReceivePinnedBuffer = default(GCHandle);
}

_operationHandle.Free();
_operationHandle = default(GCHandle);
}
}

Expand Down
22 changes: 1 addition & 21 deletions src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ internal class WinHttpResponseStream : Stream
private readonly WinHttpRequestState _state;
private SafeWinHttpHandle _requestHandle;

// TODO (Issue 2505): temporary pinned buffer caches of 1 item. Will be replaced by PinnableBufferCache.
private GCHandle _cachedReceivePinnedBuffer = default(GCHandle);

internal WinHttpResponseStream(SafeWinHttpHandle requestHandle, WinHttpRequestState state)
{
_state = state;
Expand Down Expand Up @@ -120,16 +117,7 @@ public override Task<int> ReadAsync(byte[] buffer, int offset, int count, Cancel
throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
}

// TODO (Issue 2505): replace with PinnableBufferCache.
if (!_cachedReceivePinnedBuffer.IsAllocated || _cachedReceivePinnedBuffer.Target != buffer)
{
if (_cachedReceivePinnedBuffer.IsAllocated)
{
_cachedReceivePinnedBuffer.Free();
}

_cachedReceivePinnedBuffer = GCHandle.Alloc(buffer, GCHandleType.Pinned);
}
_state.PinReceiveBuffer(buffer);

_state.TcsReadFromResponseStream =
new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously);
Expand Down Expand Up @@ -218,14 +206,6 @@ protected override void Dispose(bool disposing)
{
_disposed = true;

// TODO (Issue 2508): Pinned buffers must be released in the callback, when it is guaranteed no further
// operations will be made to the send/receive buffers.
if (_cachedReceivePinnedBuffer.IsAllocated)
{
_cachedReceivePinnedBuffer.Free();
_cachedReceivePinnedBuffer = default(GCHandle);
}

if (disposing)
{
if (_requestHandle != null)
Expand Down
38 changes: 2 additions & 36 deletions src/System.Net.WebSockets.Client/src/System/Net/WebSockets/WinHttpWebSocket.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ internal class WinHttpWebSocket : WebSocket

private WinHttpWebSocketState _operation = new WinHttpWebSocketState();

// TODO (Issue 2505): temporary pinned buffer caches of 1 item. Will be replaced by PinnableBufferCache.
private GCHandle _cachedSendPinnedBuffer;
private GCHandle _cachedReceivePinnedBuffer;

public WinHttpWebSocket()
{
}
Expand Down Expand Up @@ -299,16 +295,7 @@ public override Task SendAsync(
{
var bufferType = WebSocketMessageTypeAdapter.GetWinHttpMessageType(messageType, endOfMessage);

// TODO (Issue 2505): replace with PinnableBufferCache.
if (!_cachedSendPinnedBuffer.IsAllocated || _cachedSendPinnedBuffer.Target != buffer.Array)
{
if (_cachedSendPinnedBuffer.IsAllocated)
{
_cachedSendPinnedBuffer.Free();
}

_cachedSendPinnedBuffer = GCHandle.Alloc(buffer.Array, GCHandleType.Pinned);
}
_operation.PinSendBuffer(buffer);

bool sendOperationAlreadyPending = false;
if (_operation.PendingWriteOperation == false)
Expand Down Expand Up @@ -367,16 +354,7 @@ public override async Task<WebSocketReceiveResult> ReceiveAsync(

using (CancellationTokenRegistration ctr = ThrowOrRegisterCancellation(cancellationToken))
{
// TODO (Issue 2505): replace with PinnableBufferCache.
if (!_cachedReceivePinnedBuffer.IsAllocated || _cachedReceivePinnedBuffer.Target != buffer.Array)
{
if (_cachedReceivePinnedBuffer.IsAllocated)
{
_cachedReceivePinnedBuffer.Free();
}

_cachedReceivePinnedBuffer = GCHandle.Alloc(buffer.Array, GCHandleType.Pinned);
}
_operation.PinReceiveBuffer(buffer);

await InternalReceiveAsync(buffer).ConfigureAwait(false);

Expand Down Expand Up @@ -752,18 +730,6 @@ public override void Dispose()
{
_operation.Dispose();

// TODO (Issue 2508): Pinned buffers must be released in the callback, when it is guaranteed no further
// operations will be made to the send/receive buffers.
if (_cachedReceivePinnedBuffer.IsAllocated)
{
_cachedReceivePinnedBuffer.Free();
}

if (_cachedSendPinnedBuffer.IsAllocated)
{
_cachedSendPinnedBuffer.Free();
}

_disposed = true;
}
}
Expand Down
45 changes: 45 additions & 0 deletions src/System.Net.WebSockets.Client/src/System/Net/WebSockets/WinHttpWebSocketState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ internal sealed class WinHttpWebSocketState : IDisposable
private volatile bool _pendingReadOperation = false;
private volatile bool _pendingWriteOperation = false;

// TODO (Issue 2505): temporary pinned buffer caches of 1 item. Will be replaced by PinnableBufferCache.
private GCHandle _cachedSendPinnedBuffer = default(GCHandle);
private GCHandle _cachedReceivePinnedBuffer = default(GCHandle);

private volatile bool _disposed = false; // To detect redundant calls

public WinHttpWebSocketState()
Expand All @@ -56,11 +60,52 @@ public void Unpin()
{
Debug.Assert(_handlesOpenWithCallback == 0);

// This method only gets called when the WinHTTP request/websocket handles are fully closed and thus
// all async operations are done. So, it is safe at this point to unpin the buffers and release
// the strong GCHandle for this object.
if (_cachedReceivePinnedBuffer.IsAllocated)
{
_cachedReceivePinnedBuffer.Free();
_cachedReceivePinnedBuffer = default(GCHandle);
}

if (_cachedSendPinnedBuffer.IsAllocated)
{
_cachedSendPinnedBuffer.Free();
_cachedSendPinnedBuffer = default(GCHandle);
}

_operationHandle.Free();
_operationHandle = default(GCHandle);
}
}

public void PinSendBuffer(ArraySegment<byte> buffer)
{
if (!_cachedSendPinnedBuffer.IsAllocated || _cachedSendPinnedBuffer.Target != buffer.Array)
{
if (_cachedSendPinnedBuffer.IsAllocated)
{
_cachedSendPinnedBuffer.Free();
}

_cachedSendPinnedBuffer = GCHandle.Alloc(buffer.Array, GCHandleType.Pinned);
}
}

public void PinReceiveBuffer(ArraySegment<byte> buffer)
{
if (!_cachedReceivePinnedBuffer.IsAllocated || _cachedReceivePinnedBuffer.Target != buffer.Array)
{
if (_cachedReceivePinnedBuffer.IsAllocated)
{
_cachedReceivePinnedBuffer.Free();
}

_cachedReceivePinnedBuffer = GCHandle.Alloc(buffer.Array, GCHandleType.Pinned);
}
}

public void IncrementHandlesOpenWithCallback()
{
Interlocked.Increment(ref _handlesOpenWithCallback);
Expand Down