Avoid a closure allocation in MessageProcessingHandler

[jamesqo]

Contributes to #7787 by removing a closure allocation from MessageProcessingHandler.SendAsync.

cc @davidsh @stephentoub

[jamesqo]

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.

[stephentoub]

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.

[justinvp]

Could this just be refactored to use async/await, ensuring the same behavior is maintained, along the lines of #7464? Otherwise, the Tuple allocation could be avoided by using a private subclass of TaskCompletionSource<HttpResponseMessage> to carry the state.

[stephentoub]

This one is harder to refactor with async/await, specifically because it's distinguishing between OperationCanceledExceptions with and without a matching token, translating some to Canceled and some to Faulted.

[jamesqo]

@justinvp Thank you for the PR feedback, I had not thought of that. It also results in much cleaner code 😄

[justinvp]

Nit: sealed

[justinvp]

Nit: As a matter of personal style, I would have defined this at the bottom of the containing class, but I don't think there's a guideline one way or the other.

[justinvp]

Would something like SendCompletionSource be a better name for this?

[jamesqo]

@justinvp I followed @stephentoub's precedent here in CurlHandler for this class, which didn't use sealed and didn't have a CompletionSource in the class name. In hindsight though, it looks like CancelableReadState (which extends ReadState) is sealed since it doesn't have any subclasses, so I guess you're right in that aspect.

[justinvp]

@jamesqo, the credit for the TaskCompletionSource<TResult>-subclass idea goes to @stephentoub as I remember him suggesting or using it in the past. 😄

[justinvp]

Would something like Handler be a better name for this (if so, update the ctor parameter to match)?

[jamesqo]

You're right, self is kind of confusing since it might refer to the handler or SendState itself. Will update.

[jamesqo]

@stephentoub and @davidsh, can you please re-review and see if this is ready to be merged?

[davidsh]

SendState -> var

[davidsh]

@jamesqo I added one comment. Please address that and then we'll be ok to merge when green. We'll also want to squash the commits. Thanks for your work on this.

[stephentoub]

LGTM2

[jamesqo]

@davidsh Feedback has been addressed. FYI, GitHub has recently added a new feature that allows collaborators to squash the commits themselves while merging (e.g. @stephentoub just did this with my PR in #7468), so I believe it should be possible to squash without my help.