Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bf-streaming] Web Socket disconnected should rejects all ongoing requests #4452

Closed
compulim opened this issue Apr 21, 2023 · 0 comments · Fixed by #4461
Closed

[bf-streaming] Web Socket disconnected should rejects all ongoing requests #4452

compulim opened this issue Apr 21, 2023 · 0 comments · Fixed by #4461
Labels
bug Indicates an unexpected problem or an unintended behavior. needs-triage The issue has just been created and it has not been reviewed by the team.

Comments

@compulim
Copy link
Contributor

compulim commented Apr 21, 2023

Github issues should be used for bugs and feature requests. Use Stack Overflow for general "how-to" questions.

Versions

What package version of the SDK are you using.
4.19.3

What nodejs version are you using
18

What browser version are you using
No

What os are you using
Ubuntu

Describe the bug

RequestManager keep tracks of all outgoing requests. More-or-less the call manager of RPC.

When a Web Socket disconnected, botframework-streaming does not signal RequestManager to reject all pending requests. Thus, all pending requests are kept forever and never returned.

One failing case, after Web Socket disconnected, C2 can still send an activity. For that send request, despite it is disconnected and should fail, were never rejected and just keep on forever.

To Reproduce

Steps to reproduce the behavior:

  1. Instantiate DirectLineStreaming via botframework-directlinejs package
  2. Call postActivity()
  3. Before the message is put on the Web Socket connection, kill the connection

Expected behavior

postActivity() should fail.

However, it is not failing because NodeWebSocketClient and BrowserWebSocketClient did not tell RequestManager about the disconnection.

While we can say NodeWebSocketClient/BrowserWebSocketClient should signal disconnection, however, RequestManager has no public members to reject all pending requests.

Even worse, the requests at RequestManager._pendingRequests are not rejectable. There are no way to call pendingRequest.reject() function.

That means, no requests can be rejected regardless of whatever situation. This means, either a request resolved, or keep on forever.

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

Add any other context about the problem here.

@compulim compulim added bug Indicates an unexpected problem or an unintended behavior. needs-triage The issue has just been created and it has not been reviewed by the team. labels Apr 21, 2023
@compulim compulim changed the title [Streaming] Web Socket disconnected should rejects all ongoing requests [bf-streaming] Web Socket disconnected should rejects all ongoing requests Apr 21, 2023
compulim added a commit that referenced this issue Apr 24, 2023
…ing requests on disconnection (#4461)

* Fix inconclusive tests

* Rejects pending requests on disconnection

* isConnected should always return boolean

* Fix race condition of missed responses

* Add client/server tests

* Skip test if it is not on Windows

* Fix for Node.js 12

* Fix and skip failing tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or an unintended behavior. needs-triage The issue has just been created and it has not been reviewed by the team.
Projects
None yet
1 participant