Botbuilder-dotnet: State manager only persists context state if there is a response

[JonathanFingold]

Current implementation
The state manager persists state as part of the send activity pipeline.
The bot runs the send activity pipeline only when there's at least one response in the context object.

Issue
When there is a state change, it is only persisted if there is at least one accompanying response.

[RyanDawkins]

Looks like this is the case because the BotStateManager is implemented in Middleware. Currently the bot state is saved during the ISendActivity.SendActivity method.

https://github.com/Microsoft/botbuilder-dotnet/blob/master/libraries/Microsoft.Bot.Builder/BotStateManager.cs#L66

[RyanDawkins]

Working on a change for this right now. Got it working so that it persists state, however I'm going to create some unit tests first.

[cleemullins]

It'll be interesting to see where you decide to put the state persistence. I'll need to make sure the Node SDK is updated to work the same way, as behavior like that needs to be consistent across the 2 platforms.

Tests around this will make ALL the difference! Hopefully there is enough scaffolding already in place that writing the tests isn't too hard, and doesn't require extensive mocks.

[RyanDawkins]

After putting more thought into this, I can think of three different approaches to how we can handle this.

1. Remove check for an empty list of activities before _middlewareSet.SendActivity is called. This was my initial approach to make sure the middleware is called.
2. When Runpipeline is called check if IBotContext.Responses is empty. If the responses list is empty, add a new "NoOp" activity type. This would make it clear to our middleware users know that no response was sent from the Bot and there were no errors.
3. Create a new middleware event that signals the Pipeline finishing. This would be triggered after await SendActivity. No responses would be sent, just IBotContext.

The first option is just changing Bot.cs#L75 from: if (context.Responses != null && context.Responses.Any()) to if (context.Responses != null)

[cleemullins]

Another option would be to rename the pipeline to just "AfterReceive" and run it in all cases. This would allow other similar middleware (analytics, counting, or whatever) to run.

I think the semantics of "only run the pipe if there's an activity in the send queue" is simply incorrect.

Let me think about it for a day. I would love to hear opinions from others on this...

[RyanDawkins]

Yeah I think the rename may be the best option and allow no activities.

[Stevenic]

So the JavaScript SDK currently saves even if you don't send any activities. That's because I always call flushResponses() just before destroying the context and that causes a trip through the postActivity() pipe with an empty activity array which I agree sounds a little yucky but works.

For this, and other, reasons I've suggested that we consolidate the 3 current pipes we have into a single outer pipe that lets you essentially subscribe to events off the context object. This change would make it super clear that state reads occur on the leading edge of the ContextCreated() and that the changes are then written back out on the trailing edge of ContextCreated(). In pseudo code form this looks like:

// Read state
await context.state = storage.read(key);

// Call next
await next();

// Write state
await storage.write(key, context.state);

[cleemullins]

Addressed (for the short term) in PR #158.

I changed the code to always run the Send() pipeline, even if there's nothing to send. This makes sense, as this is more of a post-processing step, and actually sending activities is simply one-of-many post processing steps.

I believe we will eventually end up with the "Single Pipeline" solution, but this still seems a bug worth fixing.

[Stevenic]

For what its worth @cleemullins that's what I currently do as well

[cleemullins]

Fixed.