Request aborted #335
Request aborted #335
Conversation
…DisposalCloses test
|
Hi @tmds, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
@tmds, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| "Microsoft.AspNet.Http.Abstractions": "1.0.0-*", | ||
| "Microsoft.AspNet.FileProviders.Abstractions": "1.0.0-*", | ||
| "Microsoft.Framework.NotNullAttribute.Sources": { "type": "build", "version": "1.0.0-*" } | ||
| "Microsoft.AspNet.Http.Abstractions": "1.0.0-beta6", |
There was a problem hiding this comment.
Please go back to *, we're working on beta8 now.
|
lock.json files should not be checked in. |
|
Most of these changes are noise because your branch is based on master and you're trying to merge into dev. please rebase your changes onto dev directly. |
…DisposalCloses test
| { | ||
| public static class HttpResponseMessageExtensions | ||
| { | ||
| public static Task PipelineCompleteAsync(this HttpResponseMessage message) |
There was a problem hiding this comment.
This seems overkill. Why can't the client just wait for the end of the response stream?
There was a problem hiding this comment.
I agree this is not necessary for implementing RequestAborted.
The test I was planning to write using RequestAborted has an effect on subsequent requests. I noticed the effects weren't visible immediately because when the ResponseStream returns, the pipeline hadn't finished yet.
I think it is an interesting feature independent of RequestAborted.
If you like, I can leave it out here and open another feature request?
There was a problem hiding this comment.
Why is the response stream completing before the end of the pipeline? Is it checking content-length? Or did somebody call Dispose on it?
There was a problem hiding this comment.
The test called Dispose on it to abort the request.
There was a problem hiding this comment.
I think this is more complicated than we want the test client to be. We want HttpClient to be used like it normally would. If you really need to control the server data structures and flow then you can call TestServer.Invoke directly.
…o request_aborted Conflicts: src/Microsoft.AspNet.Hosting.Server.Abstractions/project.json src/Microsoft.AspNet.Hosting/project.json src/Microsoft.AspNet.Server.Testing/project.json src/Microsoft.AspNet.TestHost/ClientHandler.cs src/Microsoft.AspNet.TestHost/project.json test/Microsoft.AspNet.Hosting.Tests/project.json test/Microsoft.AspNet.TestHost.Tests/project.json
| @@ -7,6 +7,8 @@ | |||
| using Microsoft.AspNet.Builder; | |||
| using Microsoft.AspNet.Http; | |||
| using Xunit; | |||
| using System.Threading; | |||
…ived class of OperationCanceledException may also be thrown (TaskCanceledException)
| { | ||
| 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.
This incoming CT needs to be linked to _requestAbortedSource.
There was a problem hiding this comment.
There is a registration in SendAsync which calls AbortRequest, which then Cancels _requestAbortedSource. AbortRequest also calls Complete on the _responseStream.
| _responseStream.Complete(); | ||
| } | ||
|
|
||
| internal void PipelineComplete() |
There was a problem hiding this comment.
Method names should be verbs/actions. It's not clear why you renamed most of these methods.
There was a problem hiding this comment.
Renames where part of PipelineCompleteAsync feature, which has been removed.
|
I synced your branch and I finally understand what's wrong with your history. Your source tree is a merger of dev and master which causes a big mess. The PR needs to be based on the dev branch with no mergers. I tried cherry picking things over to dev but ran into a bunch of conflicts. Try resetting to dev and pulling your changes over by source. That will help you squash all the commits in the process. |
|
Replaced by #345 |
Support RequestAborted (fixes #320)
Add PipelineCompleteAsync to wait for pipeline operations for a request to complete.
Code based on 1.0.0-beta6 branch