Provide Cleanup Guidance around closing websockets

[cleemullins]

Originally reported in #1655, and moved here so we've got an issue tracking the ask.

As there's not a single BotFrameworkAdapter but one for every WebSocket connection, it would also mean the new SkillHandler as well as ChannelServiceRoutes have to exist multiple times, which results in every conversation/connection resulting in a different endpoint via the SkillHandler (skillEndpoint.register(server, '/api/skills');).
Which currently is not easy to cleanup as the WebSocket does not seem to expose a disconnected event. This would result in a memory leak.

This is work we plan on shipping as part of R8, which ships ~March 15.

[alopix]

Giving feedback on the linked ticket:
The setup is basically a combination of the two samples:

• https://github.com/microsoft/BotBuilder-Samples/blob/master/experimental/directline-speech/javascript_nodejs/02.echo-bot/index.js
• https://github.com/microsoft/BotBuilder-Samples/blob/master/samples/javascript_nodejs/80.skills-simple-bot-to-bot/simple-root-bot/index.js

server.on('upgrade', async (req, socket, head) => {
    // Create an adapter scoped to this WebSocket connection to allow storing session data.
    const streamingAdapter = new BotFrameworkAdapter(adapterSettings);
    // Set onTurnError for the BotFrameworkAdapter created for each connection.
    streamingAdapter.onTurnError = onTurnErrorHandler;

    await streamingAdapter.useWebSocket(req, socket, head, async (context) => {
        // After connecting via WebSocket, run this logic for every request sent over
        // the WebSocket connection.
        await myBot.run(context);
    });
});

combined with (both top-level)

// Create and initialize the skill classes
const authConfig = new AuthenticationConfiguration([], allowedSkillsClaimsValidator);
const handler = new SkillHandler(adapter, bot, conversationIdFactory, credentialProvider, authConfig);
const skillEndpoint = new ChannelServiceRoutes(handler);
skillEndpoint.register(server, '/api/skills');

Would not work, as SkillHandler requires access to the adapter, which is only created on connection -> ChannelServiceRoutes and registration with the server would have to be inside the websocket connection handler as well, so requires a different endpoint all the time.

[johnataylor]

From another thread:

This is a problem that occurs when custom adapters that abstract http request-reply protocols are combined with Skills that use the Bot Framework http based "inline proactive" protocol where replies are returned to a caller by initiating a new secondary http call in the opposite direction.

Examples of custom adapter that abstract underlying http request-reply are the Amazon Alexa and the Google Home adapters.

As the front end protocol is request-reply when the call is forwarded to another party (as is the case with a Skill) it makes sense to do this as a request-reply call with its intrinsic network affinity, rather than splitting the call into what would be separate http calls initiated in opposite directions.

The proposal is to add a value of RequestReply to the DeliveryMode field of an Activity that indicates that parties that need to forward the Activity should do so with a conventional http request and then expect any reply Activities on that request's reply.

Alternative designs considered include making the Activity the value of an InvokeActivity which already has this network binding by default. However, this design has the disadvantage of requiring more code further up the stack, particularly around making it so the extra nesting is hidden from the application layer. Overall using the DeliveryMode is the simpler design.

[cleemullins]

Blocked until Big Rocks closed on the path forward.

[cleemullins]

The design discussion(S) around this close today, so we should be unblocked tomorrow (Feb 26).