search

microsoft / botbuilder-dotnet

Welcome to the Bot Framework SDK for .NET repository, which is the home for the libraries and packages that enable developers to build sophisticated bot applications using .NET.
https://github.com/Microsoft/botframework
MIT License
878 stars 484 forks source link

NotSupportedException in BotMessageHandler at request.Body.Position = 0 #1371

Closed EricDahlvang closed 5 years ago

EricDahlvang commented 5 years ago

Version

Pulled this branch: CLM/CosmosDBKeyLength to test against a CosmosDb instance for: https://github.com/Microsoft/botbuilder-dotnet/pull/1370

Describe the bug

System.NotSupportedException: Specified method is not supported. in Microsoft.Bot.Builder.Integration.AspNet.Core.Handlers.BotMessageHandler on

request.Body.Position = 0;
System.NotSupportedException: Specified method is not supported.
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.set_Position(Int64 value)
   at Microsoft.Bot.Builder.Integration.AspNet.Core.Handlers.BotMessageHandler.ProcessMessageRequestAsync(HttpRequest request, IAdapterIntegration adapter, BotCallbackHandler botCallbackHandler, CancellationToken cancellationToken) in C:\work\v4\2.8.2019\botbuilder-dotnet\libraries\integration\Microsoft.Bot.Builder.Integration.AspNet.Core\BotMessageHandler.cs:line 23
   at Microsoft.Bot.Builder.Integration.AspNet.Core.Handlers.BotMessageHandlerBase.HandleAsync(HttpContext httpContext) in C:\work\v4\2.8.2019\botbuilder-dotnet\libraries\integration\Microsoft.Bot.Builder.Integration.AspNet.Core\BotMessageHandlerBase.cs:line 60
   at Microsoft.AspNetCore.Builder.Extensions.MapMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

To Reproduce

Steps to reproduce the behavior:

  1. Run Microsoft.Bot.Builder.TestBot
  2. Connect the emulator
  3. Send a message to the bot
  4. See error

Expected behavior

No exception

Screenshots

If applicable, add screenshots to help explain your problem. image

Additional context

I'm not sure if this is due to: https://github.com/Microsoft/botbuilder-dotnet/commit/4a4ccd74fe650ddd97570156e2ca964d6155db69 ... or if the Microsoft.Bot.Builder.TestBot is using the wrong version of some library?

[bug]

drub0y commented 5 years ago

Ah, so clearly there wasn't a test done on a non-buffered Stream. We can't let the code blindly reposition like this because, if buffering wasn't enabled, the Stream is forward only 'cause it's literally streaming the bytes off the network, so a call to set the Position is not supported.

A check to Stream::CanSeek should fix this problem. Testing it out now and will PR.

drub0y commented 5 years ago

Confirmed the above. Working on full fix and want to make sure a buffering stream still works as well. PR incoming soon.

EricDahlvang commented 5 years ago

@drub0y I don't know enough about how this all works. Is this same issue also going to be a problem here:

https://github.com/Microsoft/botbuilder-dotnet/blob/master/libraries/integration/Microsoft.Bot.Builder.Integration.ApplicationInsights.Core/TelemetrySaveBodyASPMiddleware.cs#L49

Or does this .EnableBuffering call handle it? https://github.com/Microsoft/botbuilder-dotnet/blob/master/libraries/integration/Microsoft.Bot.Builder.Integration.ApplicationInsights.Core/TelemetrySaveBodyASPMiddleware.cs#L30

It seems like we should be setting a higher than default bufferThreshold when calling EnableBuffering.

drub0y commented 5 years ago

@EricDahlvang that's right, the EnableBuffering call happens 100% of the time there so that makes it "safe" for multiple reads and, unlike the Sentry implementation discussed in this issue, the TelemetrySaveBodyASPMiddleware was never trying to read the body after the BotMessageHandler, so it didn't run into the same issues due to BotMessageHandler closing the Stream.