-
Notifications
You must be signed in to change notification settings - Fork 311
Request aborted #335
Request aborted #335
Changes from 16 commits
56653a5
ad7b030
e7ade98
2cd343e
96972bd
a1743cc
a12863a
ba46918
9afb17e
12d7182
19a9b86
fad9dc9
17535b8
696c905
677a7cb
72906a9
e88c503
fc4c331
cfd11dc
9be8be3
f4bcdb9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<configuration> | ||
<packageSources> | ||
<add key="AspNetVNext" value="https://www.myget.org/F/aspnetvnext/api/v2" /> | ||
<add key="NuGet" value="https://nuget.org/api/v2/" /> | ||
<add key="AspNetVNext" value="https://www.myget.org/F/aspnetlitedev/api/v2" /> | ||
<add key="NuGet" value="https://api.nuget.org/v3/index.json" /> | ||
</packageSources> | ||
</configuration> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,41 +62,44 @@ protected override async Task<HttpResponseMessage> SendAsync( | |
body.Seek(0, SeekOrigin.Begin); | ||
} | ||
state.HttpContext.Request.Body = body; | ||
var registration = cancellationToken.Register(state.Abort); | ||
var registration = cancellationToken.Register(state.AbortRequest); | ||
|
||
// Async offload, don't let the test code block the caller. | ||
var offload = Task.Factory.StartNew(async () => | ||
{ | ||
try | ||
{ | ||
await _next(state.FeatureCollection); | ||
state.CompleteResponse(); | ||
state.PipelineComplete(); | ||
} | ||
catch (Exception ex) | ||
{ | ||
state.Abort(ex); | ||
state.PipelineFailed(ex); | ||
} | ||
finally | ||
{ | ||
registration.Dispose(); | ||
state.Dispose(); | ||
} | ||
}); | ||
|
||
return await state.ResponseTask; | ||
} | ||
|
||
private class RequestState : IDisposable | ||
private class RequestState | ||
{ | ||
private readonly HttpRequestMessage _request; | ||
private TaskCompletionSource<HttpResponseMessage> _responseTcs; | ||
private ResponseStream _responseStream; | ||
private ResponseFeature _responseFeature; | ||
private CancellationTokenSource _requestAbortedSource; | ||
private bool _pipelineFinished; | ||
|
||
internal RequestState(HttpRequestMessage request, PathString pathBase, CancellationToken cancellationToken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This incoming CT needs to be linked to _requestAbortedSource. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nevermind, that's on line 65 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a registration in SendAsync which calls AbortRequest, which then Cancels _requestAbortedSource. AbortRequest also calls Complete on the _responseStream. |
||
{ | ||
_request = request; | ||
_responseTcs = new TaskCompletionSource<HttpResponseMessage>(); | ||
_requestAbortedSource = new CancellationTokenSource(); | ||
_pipelineFinished = false; | ||
|
||
if (request.RequestUri.IsDefaultPort) | ||
{ | ||
|
@@ -146,9 +149,10 @@ internal RequestState(HttpRequestMessage request, PathString pathBase, Cancellat | |
} | ||
} | ||
|
||
_responseStream = new ResponseStream(CompleteResponse); | ||
_responseStream = new ResponseStream(ReturnResponseMessage, AbortRequest); | ||
HttpContext.Response.Body = _responseStream; | ||
HttpContext.Response.StatusCode = 200; | ||
HttpContext.RequestAborted = _requestAbortedSource.Token; | ||
} | ||
|
||
public HttpContext HttpContext { get; private set; } | ||
|
@@ -160,7 +164,28 @@ public Task<HttpResponseMessage> ResponseTask | |
get { return _responseTcs.Task; } | ||
} | ||
|
||
internal void CompleteResponse() | ||
public CancellationToken RequestAborted | ||
{ | ||
get { return _requestAbortedSource.Token; } | ||
} | ||
|
||
public void AbortRequest() | ||
{ | ||
if (!_pipelineFinished) | ||
{ | ||
_requestAbortedSource.Cancel(); | ||
} | ||
_responseStream.Complete(); | ||
} | ||
|
||
internal void PipelineComplete() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Method names should be verbs/actions. It's not clear why you renamed most of these methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renames where part of PipelineCompleteAsync feature, which has been removed. |
||
{ | ||
_pipelineFinished = true; | ||
ReturnResponseMessage(); | ||
_responseStream.Complete(); | ||
} | ||
|
||
internal void ReturnResponseMessage() | ||
{ | ||
if (!_responseTcs.Task.IsCompleted) | ||
{ | ||
|
@@ -173,7 +198,7 @@ internal void CompleteResponse() | |
|
||
[SuppressMessage("Microsoft.Reliability", "CA2000:DisposeObjectsBeforeLosingScope", | ||
Justification = "HttpResposneMessage must be returned to the caller.")] | ||
internal HttpResponseMessage GenerateResponse() | ||
private HttpResponseMessage GenerateResponse() | ||
{ | ||
_responseFeature.FireOnSendingHeaders(); | ||
|
||
|
@@ -196,22 +221,12 @@ internal HttpResponseMessage GenerateResponse() | |
return response; | ||
} | ||
|
||
internal void Abort() | ||
{ | ||
Abort(new OperationCanceledException()); | ||
} | ||
|
||
internal void Abort(Exception exception) | ||
internal void PipelineFailed(Exception exception) | ||
{ | ||
_pipelineFinished = true; | ||
_responseStream.Abort(exception); | ||
_responseTcs.TrySetException(exception); | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
_responseStream.Dispose(); | ||
// Do not dispose the request, that will be disposed by the caller. | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,8 @@ | |
using Microsoft.AspNet.Builder; | ||
using Microsoft.AspNet.Http; | ||
using Xunit; | ||
using System.Threading; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ordering, System goes first. |
||
using System; | ||
|
||
namespace Microsoft.AspNet.TestHost | ||
{ | ||
|
@@ -111,5 +113,38 @@ public async Task PostAsyncWorks() | |
// Assert | ||
Assert.Equal("Hello world POST Response", await response.Content.ReadAsStringAsync()); | ||
} | ||
|
||
[Fact] | ||
public async Task ClientDisposalAbortsRequest() | ||
{ | ||
// Arrange | ||
TaskCompletionSource<object> tcs = new TaskCompletionSource<object>(); | ||
RequestDelegate appDelegate = async ctx => | ||
{ | ||
// Write Headers | ||
await ctx.Response.Body.FlushAsync(); | ||
|
||
var sem = new SemaphoreSlim(0); | ||
try | ||
{ | ||
await sem.WaitAsync(ctx.RequestAborted); | ||
} | ||
catch(Exception e) | ||
{ | ||
tcs.SetException(e); | ||
} | ||
}; | ||
|
||
// Act | ||
var server = TestServer.Create(app => app.Run(appDelegate)); | ||
var client = server.CreateClient(); | ||
var request = new HttpRequestMessage(HttpMethod.Get, "http://localhost:12345"); | ||
var response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead); | ||
// Abort Request | ||
response.Dispose(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a separate test for aborting a request before the response is received. You could do this with a CancellationToken or just dispose the HttpClient. |
||
|
||
// Assert | ||
var exception = await Assert.ThrowsAsync<OperationCanceledException>(async () => await tcs.Task); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's still something odd with your rebase if you are including changes in this file.