Botbuilder-js: ShowTypingMiddleware suppresses errors and does not allow adapter.onTurnError to handle them

[jovanmilenkoski]

Versions

What package version of the SDK are you using.
4.2.0
What nodejs version are you using
8.11.1
What browser version are you using
N/A
What os are you using
MacOS

Describe the bug

I have a onTurnError handler and ShowTypingMiddleware attached to the botframework adapter.

    adapter = new BotFrameworkAdapter(adapterSettings)
        .use(new ShowTypingMiddleware());

    // Catch-all for errors.
    adapter.onTurnError = onTurnError;

I've done some "digging" in the source code and found out this line

return await next().then(stopInterval, stopInterval);

which just stops the interval in error/success scenario and does not allows the error to propagate to the adapter which normally "fires" onTurnError handler function.

The fix:

function stopInterval(): void {
    finished = true;
    if (hTimeout) { clearTimeout(hTimeout); }
}

function stopIntervalWithError(err: Error): void {
    stopInterval();
    throw err; //  here the error is re-thrown
}
return await next().then(stopInterval, stopIntervalWithError);

To Reproduce

Steps to reproduce the behavior:
N/A

Expected behavior

BotFrameworkAdapter to "fire" the provided onTurnError function.

I would gladly contribute with a pull request, but following the guidelines https://github.com/Microsoft/botbuilder-js/wiki/Contribution-Guidelines

when I try to push the new created branch, I encounter the following

git push -u origin fix/typing-middleware-suppresses-errors
ERROR: Permission to Microsoft/botbuilder-js.git denied to jovanmilenkoski.

[bug]

[sgellock]

This is by design. @johnataylor please add additional context and reasoning

[johnataylor]

This behavior is by design.

A typing indicator is just a visual hint to the user sent across the channel and not part of the core bot logic. No business logic is impact if the message (the typing activity) is dropped.

The scenario for using the error handler is when something like state changes fail or perhaps a call to a back end service or something like that that would impact the bots business logic.

[drub0y]

I don't think this is by design.

This bug shows that use of this middleware prevents an error, happening anywhere in the pipeline from it forward (especially the bot's onTurn handler), from propagating to the configured turn error handler. Surely we want those errors to propagate?

The proposed fix seems perfectly valid to me.

[esoler-sage]

Hi @johnataylor the problem is that if some issue happens in the middle of the conversation, the typing middleware supresses the error (so you loose all the tracking information like the onTurnError event Can you reopen the issue or do we need to open a new one?

[EricDahlvang]

Re-opened, R8, @johnataylor