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

Avoid a closure allocation in MessageProcessingHandler #7788

Merged
davidsh merged 5 commits into from
Apr 23, 2016
Merged

Avoid a closure allocation in MessageProcessingHandler #7788

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7bccae8
Avoid a closure allocation in MessageProcessingHandler
jamesqo Apr 16, 2016
6eb94a0
Respond to PR feedback: Introduce SendState private class
jamesqo Apr 16, 2016
980d674
Respond to PR feedback, #2
jamesqo Apr 16, 2016
2e4788b
public -> internal in SendState
jamesqo Apr 20, 2016
1790fd4
SendState -> var
jamesqo Apr 22, 2016
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
22 changes: 14 additions & 8 deletions src/System.Net.Http/src/System/Net/Http/MessageProcessingHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,40 +47,46 @@ protected internal sealed override Task<HttpResponseMessage> SendAsync(HttpReque

// We schedule a continuation task once the inner handler completes in order to trigger the response
// processing method. ProcessResponse() is only called if the task wasn't canceled before.
sendAsyncTask.ContinueWithStandard(task =>
var tuple = Tuple.Create(this, tcs, cancellationToken);
Copy link
Contributor Author

@jamesqo jamesqo Apr 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Note: I know the coding guidelines say to not use var if the type isn't obvious, but I intentionally left it out here. The full type is

Tuple<MessageProcessingHandler, TaskCompletionSource<HttpResponseMessage>, CancellationToken>

which is pretty long IMO. If any of you want, I can put the Tuple.Create(...) method call directly within the ContinueWithStandard expression.

Copy link
Member

@stephentoub stephentoub Apr 16, 2016

Choose a reason for hiding this comment

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

It may be long, but it's also important for type safety and catching things early. If, for example, someone were to inadvertently change one of the arguments to Create, it would still compile fine, and it would only fail at run time if/when that object was cast inside of the continuation. It be much better if such a change failed at compile time due to the types not matching up, and one way to get that is by not using var here. So please follow the guidelines. Thanks.

sendAsyncTask.ContinueWithStandard(tuple, (task, state) =>
{
var t = (Tuple<MessageProcessingHandler, TaskCompletionSource<HttpResponseMessage>, CancellationToken>)state;
MessageProcessingHandler self = t.Item1;
TaskCompletionSource<HttpResponseMessage> source = t.Item2;
CancellationToken token = t.Item3;

if (task.IsFaulted)
{
tcs.TrySetException(task.Exception.GetBaseException());
source.TrySetException(task.Exception.GetBaseException());
return;
}

if (task.IsCanceled)
{
tcs.TrySetCanceled();
source.TrySetCanceled();
return;
}

if (task.Result == null)
{
tcs.TrySetException(new InvalidOperationException(SR.net_http_handler_noresponse));
source.TrySetException(new InvalidOperationException(SR.net_http_handler_noresponse));
return;
}

try
{
HttpResponseMessage responseMessage = ProcessResponse(task.Result, cancellationToken);
tcs.TrySetResult(responseMessage);
HttpResponseMessage responseMessage = self.ProcessResponse(task.Result, token);
source.TrySetResult(responseMessage);
}
catch (OperationCanceledException e)
{
// If ProcessResponse() throws an OperationCanceledException check whether it is related to
// the cancellation token we received from the user. If so, cancel the Task.
HandleCanceledOperations(cancellationToken, tcs, e);
HandleCanceledOperations(token, source, e);
}
catch (Exception e)
{
tcs.TrySetException(e);
source.TrySetException(e);
}
// We don't pass the cancellation token to the continuation task, since we want to get called even
// if the operation was canceled: We'll set the Task returned to the user to canceled. Passing the
Expand Down