Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.47k stars 4.8k forks source link

[BUG] `Request.Content` throws `InvalidOperationException` when we expect it should not #21048

Closed ellismg closed 3 years ago

ellismg commented 3 years ago

In a9a4ef4f9a2fb2bd19d9f452c5689df9ee98ca2e we added a .Content property to Response. The intention was that this property would make it easy for people to access the content of the response without having to deal with the underlying stream (which would include perhaps rewinding it since it is often read as part of constructing the response).

This is throwing InvalidOperationException on some platforms (net461, net50) and not others (netcoreapp3.0). We should figure out a way to fix this. Something to consider is in the case where the response is a memory stream, but the underlying buffer can not be extracted, we could copy the contents to a new BinaryData. Since the underlying data has already been copied to the MemoryBuffer, we should have to "go async" to do this.

ellismg commented 3 years ago

@JoshLove-msft mentioned that he was able to reproduce this with the service bus administration client tests.

JoshLove-msft commented 3 years ago

Looks like at least sometimes, HttpContent will use a MemoryStream constructor that does not expose the underlying buffer.

This is the code that I hit when debugging the Service Bus tests.

JoshLove-msft commented 3 years ago

/cc @minnieliu as she discovered the issue

pakrym commented 3 years ago

The memory stream exposed in our pipelines should always be created in https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/src/Pipeline/Internal/ResponseBodyPolicy.cs#L80

JoshLove-msft commented 3 years ago

The key line appears to be here - https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/src/Pipeline/Internal/ResponseBodyPolicy.cs#L66

If the stream is seekable then we don't copy to a new memory stream. I'm not sure when the stream would not be seekable.

pakrym commented 3 years ago

When it's an actual live network stream. But that might be the cause of the issue. If the HttpClient returns a memory stream without buffer available we won't rebuffer it.

pakrym commented 3 years ago

I don't know if it ever would as we use the HttpCompletionOption.ResponseHeadersRead

JoshLove-msft commented 3 years ago

If the HttpClient returns a memory stream without buffer available we won't rebuffer it.

Isn't it when the HttpClient returns a seekable memory stream, we won't rebuffer it? Maybe this depends on the size of the content being returned.

JoshLove-msft commented 3 years ago

It turned out the error was occurring in Playback mode for the Service Bus tests due to how the memory stream is constructed here - https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core.TestFramework/src/PlaybackTransport.cs#L115

It still appears to be possible to hit this when using HttpClient, but I haven't been able to repro it consistently.

pakrym commented 3 years ago

I thought you were able to repro it with HttpClient.

JoshLove-msft commented 3 years ago

I thought yuou were able to repro it with HttpClient/

I think I was able to - I actually remember stepping into the HttpContent code, but I have not been able to repro it again.