Add support for processing requests with StreamContent to AddStandardHedgingHandler()

[adamhammond]

Fixes #5105

Add support for processing HttpRequestMessage objects containing StreamContent to the AddStandardHedgingHandler() resilience API.

This change does not update any public API contracts. It updates internal and private API contracts only.

Microsoft Reviewers: Open in CodeFlow

[amcasey]

Apparently, the code coverage for M.E.Http.Resilience has dropped from 97% to 96.75% and that's why CI failed.

[joperezr]

cc: @iliar-turdushev can you take a peek?

[adamhammond]

Apparently, the code coverage for M.E.Http.Resilience has dropped from 97% to 96.75% and that's why CI failed.

@amcasey is code coverage calculated for the entire package or just for the lines I touched? I have added full test coverage for this change with the exception of the catch-block in two of the files where I catch IOException. Test coverage can't be added for this without doing something awkward like extending the StreamContent class with virtual methods so that they can be mocked. That would also require adding a certain flag on the assembly to permit this AFAIK. It doesn't seem like this is worth the trouble and added complexity. Thoughts?

[RussKie]

Apparently, the code coverage for M.E.Http.Resilience has dropped from 97% to 96.75% and that's why CI failed.

is code coverage calculated for the entire package or just for the lines I touched?

The coverage is calculated for the whole package.

You can see the build here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=649752&view=logs&j=8cf0092d-1f6c-522b-3c27-9f632d33f4f3&t=cbf2093c-af29-552b-05db-cced36272f1c ...and the report here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=649752&view=codecoverage-tab

IIUIC, the missing coverage is here:

You could also collect the coverage locally with -testcoverage switch, e.g.: .\build.cmd -build -testcoverage. See .\build.cmd -help for more info.

[RussKie]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[adamhammond]

Apparently, the code coverage for M.E.Http.Resilience has dropped from 97% to 96.75% and that's why CI failed.

is code coverage calculated for the entire package or just for the lines I touched?

The coverage is calculated for the whole package.

@RussKie I was able to add code coverage for the catch-block. I had to create a helper that extended MemoryStream with overloaded stream operation-methods to throw IOException in order to emulate the needed behavior. After adding the additional coverage, all pipeline checks passed 👍

[RussKie]

Apparently, the code coverage for M.E.Http.Resilience has dropped from 97% to 96.75% and that's why CI failed.

is code coverage calculated for the entire package or just for the lines I touched?

The coverage is calculated for the whole package.

@RussKie I was able to add code coverage for the catch-block. I had to create a helper that extended MemoryStream with overloaded stream operation-methods to throw IOException in order to emulate the needed behavior. After adding the additional coverage, all pipeline checks passed 👍

Awesome, thank you! I was about to check that our CI reports the coverage failure back to GitHub but you beat me to it with your fix :)

[martintmk]

@adamhammond

I have alternative proposal that should enable your scenarios and won't cause potential issues.

In your code-base you add the following handler:

public class BufferingHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        await request.Content.LoadIntoBufferAsync();
        return await base.SendAsync(request, cancellationToken);
    }
}

Now hedging will support StreamContent if the stream is seekable. It also gets the buffer and replicates it across hedged requests, so there are no multiple duplications. Something like:

        if (request.Content is StreamContent streamContent && await streamContent.ReadAsStreamAsync(cancellationToken) is Stream { CanSeek: true } stream)
        {
            byte[] buffer;

            if (stream is MemoryStream memoryStream)
            {
                // but buffer asside
                buffer = memoryStream.GetBuffer();
            }
            else
            {
                var memory = new MemoryStream();
                await stream.CopyToAsync(memory);
                buffer = memory.GetBuffer();
            }

            request.Content = new ByteArrayContent(buffer);
        }

This delegates the responsibility of buffering to the caller. In other words it's an explicit actions that tells us: "I have buffered the streamed content and I know what I am doing"

The only extra code on your part is that minimal BufferingHandler that you need to add before standard hedging.

WDYT?

[mobratil]

@martintmk I like your proposal because it makes the decisions about storing the stream in memory explicit. The developer should explicitly say that some potentially large allocation is going to happen. With the current PR the allocation might happen without developer's intend.

[martintmk]

Just to reiterate why I am pushing back on on this. The standard hedging is/will be used broadly across Microsoft most important services. My intuition tells me that allowing unlimited or default buffering of large streams into memory by default with eventually cause problems or outages.

So we need to do it opt-in with the option to limit the size that will be buffered.

[adamhammond]

@adamhammond

Now hedging will support StreamContent if the stream is seekable. It also gets the buffer and replicates it across hedged requests, so there are no multiple duplications. Something like:

        if (request.Content is StreamContent streamContent && await streamContent.ReadAsStreamAsync(cancellationToken) is Stream { CanSeek: true } stream)
        {
            byte[] buffer;

            if (stream is MemoryStream memoryStream)
            {
                // but buffer asside
                buffer = memoryStream.GetBuffer();
            }
            else
            {
                var memory = new MemoryStream();
                await stream.CopyToAsync(memory);
                buffer = memory.GetBuffer();
            }

            request.Content = new ByteArrayContent(buffer);
        }

WDYT?

I do like the idea of utilizing a buffer if one is available; however, this unfortunately is not possible (or rather, is not safe) to do in the context of hedging. It violates the "snapshot" pattern, breaking the implicit contract of the RequestMessageSnapshot class because reusing the buffer to clone content means that any subsequent changes made to said content in the buffer by the user after a snapshot is created will also be reflected in the snapshot.

To expand, recall that the entire purpose of RequestMessageSnapshot class is to clone HttpRequestMessage objects for the purpose of hedging. The AddStandardHedgingHandler extension method essentially adds a DelegatingHandler to the given HttpClient's handler pipeline in which it creates a resilience pipeline and executes said pipeline for the given request. One of the resilience strategies within that pipeline clones the given HttpRequestMessage into a RequestMessageSnapshot and stores it in the resilience context. The hedging strategy then uses this RequestMessageSnapshot instance to clone a new HttpRequestMessage object from the RequestMessageSnapshot object pulled from the resilience context for every subsequent (i.e. hedged) request. The important thing to note about this is that users can still add other DelegatingHandlers to the HttpClient after calling the AddStandardHedgingHandler extension method which will force said handlers to execute after the resilience handler (i.e. after the hedging strategy) in the HttpClient's handler pipeline.

To illustrate, let's say that a user added a custom DelegatingHandler that manipulates the request content in some way before sending the request over the wire. If RequestMessageSnapshot reuses the content buffer when cloning the request, then the changes made to the request content by the custom handler would also be making these changes to the snapshot's content. Let's say the request gets hedged. An HttpRequestMessage would be cloned from the snapshot; however, it wouldn't be a clone of the request prior to sending, it would be a clone of the request prior to being sent over the wire. This might result in the user making duplicate or incorrect edits when the rest of the handler pipeline executes for each hedged request.

For hedging to work properly, the RequestMessageSnapshot must be a true snapshot.

[martintmk]

For hedging to work properly, the RequestMessageSnapshot must be a true snapshot.

I agree here, let's find a way to make this work opt-in. How about adding a MaxRequestBodyBufferLength to HttpHedgingStrategyOptions or similar if the name is too much. Set it to some sensible value, 5_000_000 for example or -1 for disabled.

Then the snapshot can read this value a use it for buffering, throwing exception of buffer is exceeded.

Another alternative is to add ConfigureRequestBodyBuffering extension for IStandardHedgingBuilder. The letter being more appropriate IMHO.

[RussKie]

Hi @adamhammond I'm closing this PR as it looks like this work has stalled. If/when you decide to resume, feel free to re-open.