Adds code to leave stream open for request body #1352
Conversation
|
I don't know enough about the implications of this to have an opinion. I've tagged @drub0y (who wrote this code) to take a look. |
|
Just to add more details on this. |
There was a problem hiding this comment.
TL;DR; - this is by design and the implementation is totally appropriate. If you're using another piece of software that is reading the body before the bot, then either it or you need to enable buffering of the body for multiple reads by calling EnableBuffering or EnableRewind.
By default, for best performance, a single handler is expected to consume the body of the request. ASP.NET infrastructure will read all the headers and route the message to a handler, but the remainder of the stream is literally sitting there waiting to be read directly off the network by the handler. This offers the greatest possible performance 'cause there is no buffering of the entire request body into memory.
When multiple pieces of software need to consume the body, move through it multiple times or just use random access against it in a single request, then you need to "tell" ASP.NET you want to pay the price for this functionality. There are actually two methods provided to enable this kind of behavior EnableRewind and EnableBuffering with the latter being newer since ASP.NET Core 2.1.
I was really surprised to hear that the Sentry framework you're using doesn't do this. This is really nothing specific to bots. For example, it would have to do this to capture the JSON payload being sent to an API controller. So I did a quick search over the Sentry code base and sure enough it does do it... here it is right here (note: it uses EnableRewind). Now, it looks like it only does this if you actually configure it with the SentryAspNetCoreOptions::IncludeRequestPayload set to true. Have you set that in your application? You would need to do via the callback passed to UseSentry during startup.
|
Thanks for responding and giving me some information. |
|
This behavior is by-design, and I don't believe we should take this PR. The feedback from @drub0y seems to make this point. I'll close this out tomorrow, unless there's a compelling reason to keep it. |
|
@cleemullins So, the detail that Sentry is reading the request body AFTER the handler is fair... funky, but fair. Given this detail, let me just go do a detailed review real quick and then you can decide to take it. |
| { | ||
| activity = BotMessageHandlerBase.BotMessageSerializer.Deserialize<Activity>(bodyReader); | ||
| using (var bodyReader = new JsonTextReader(new StreamReader(request.Body, Encoding.UTF8, true, 1024, true))) |
There was a problem hiding this comment.
For these stray bool/int params that are not obvious what they are for unless you pull up intellisense (or have the entire BCL memorized 😄), please use named arguments so that it's very clear just glancing at the code. Also... I wouldn't enable detecting encoding for this scenario, so just set that to false.
new StreamReader(request.Body, Encoding.UTF8, detectEncodingFromByteOrderMarks: false, bufferSize: 1024, leaveOpen: true)| using (var bodyReader = new JsonTextReader(new StreamReader(request.Body, Encoding.UTF8))) | ||
| // Get the request body and deserialize to the Activity object. | ||
| // We need to leave the stream open here so others downstream can access it. | ||
| var originalPosition = request.Body.Position; |
There was a problem hiding this comment.
I find the whole resetting of the position for the next person to be an anti-pattern. By definition, if multiple people are going to use the stream they need to make sure they position the Stream where they want/need it. Did you experience something that lead you to do this?
If not, I would recommend the recording of the originalPosition and the try/finally.
libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/BotMessageHandler.cs
Show resolved
Hide resolved
|
Thanks for the comments. Let me know if I need to change anything else. Thanks! |
|
@odannyc This looks good, thanks! May I ask, have you actually tested these changes with Sentry in place to confirm this fully addresses the problem? I believe it should, but would feel better knowing if you have confirmed. |
|
@drub0y This is blocked on your review still. If you're good (and from comments you are), please approve your review. |
|
Yes I tested it locally and it does fix the issue. Thanks! |
A regression was introduced when support was added in #1352 to support scenario where body stream buffering was enabled. Unfortunately, during that PR, non-buffered streaming scenarios were not regression tested leading to a bug in that (the default) scenario. The bug was caused by setting `Position = 0` on the body stream when non-buffered because, non-buffered streams are not seekable and thus throw a `NotSupportedException` if you attempt to change their position.
Closes #1351