undo blocking on token polling ops; add docs & comments

[stevengum]

Fixes #4673 in 4.10 branch (will not close issue)

Specific Changes

• Change from Task.WaitAll() to Task.WhenAll() to not block background polling operations
  • This is spiritual a "reversion" of some code factoring in the 4.10.0 release
• Add comments/documentation explaining what the code is doing to prevent future breaks
• Fix minor typo

Testing

Manually tested these changes along with a fix for microsoft/BotFramework-WebChat#3439 and the workaround mentioned in #4674 with Direct Line ASE and Direct Line Speech. The OAuth flow works as expected.

Note: No magic-code OAuth flows are not supported outside of regular Direct Line with Enhanced Authentication.

[carlosscastro]

Since we are at it, it would be great to spend a bit of time making sure that any errors or timeouts in these polling tasks, there is a way for developers to detect them. Since these are ran in the background, the output status code is likely not the best avenue. However we do have a logger instance available and the ability to detect timeouts and failures in these tasks. If that seems of value, seems we could do a couple of things. 1) Catch those timeout and error scenarios. Code like the following could be in place

Task.WhenAll(pollTokenTasks.ToArray()).ContinueWith((task) =>
{
    if (task.IsCanceled)
    {
        // handle timeout logging
    }
    else if (task.IsFaulted)
    {
        // handle error, exceptions can be found in task.Exception.
    }
});

Ideally, this would be accompained by tests for timeout and fault scenario but not sure off hand how much ability we have to inject / unit test that behavior.

[stevengum]

I agree with surfacing failures to the developer and I think it's handled in the TokenResolver.PollForTokenAsync() helper (code). When I switched WaitAll() to WhenAll() I thought about adding the ContinueWith() logic from 4.9. However, in R10 the PollForTokenAsync() logic was updated so that all exceptions are caught, so I didn't think it was necessary to restore the ContinueWith()

However, I haven't tested this path in an E2E scenario, so I'm not sure if the code works according to design 🙁

@Virtual-Josh was the last person to update the tests for this class, can you confirm if we have unit test coverage for surfacing errors?

---

Regarding unit testing this class, we can stand up Moqs to send back responses for 401s, the number of 404s, 5xxs to test both the number of polling operations and non-happy paths

[carlosscastro]

Agreed on the ContinueWith, not needed because of the new try-catch.

Regarding testing, adding negative tests to this code area would be great. Not mandatory for this PR but something I'd love to see in R11 as part of our investment to make this more solid.