fix(middleware-sdk-transcribe-streaming): close sockets on WebSocketHandler.destroy() #4400
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ocketHandler to store sockets
trivikr
changed the title
trivikr
force-pushed
the
websocket-handler-destroy
branch
from
d9032e2 to
445c315
Compare
trivikr
force-pushed
the
websocket-handler-destroy
branch
from
0eb4d4d to
c54d772
Compare
trivikr
changed the title
trivikr
force-pushed
the
websocket-handler-destroy
branch
from
b83b29c to
b75951f
Compare
kuhe
reviewed
Feb 3, 2023
| /** | ||
| * Removes all closing/closed sockets from the socket pool for URL. | ||
| */ | ||
| private removeNotUsableSockets(url: string): void { |
There was a problem hiding this comment.
rename to removeClosedSockets?
There was a problem hiding this comment.
It was originally named removeClosedSockets
It removes Closing sockets too, that's why I'd renamed it from Closed to NotUsable.
kuhe
reviewed
Feb 3, 2023
| handler.destroy(); | ||
|
|
||
| // Verify that socket.close() is called | ||
| expect(socket.readyState).toBe(WebSocket.CLOSING); |
There was a problem hiding this comment.
on what timing does this transition to WebSocket.CLOSED?
There was a problem hiding this comment.
I tried timeouts of 1s and 5s, but the state was still closing.
That's why I updated source code to remove sockets with closing state, and verified state to be closing.
kuhe
approved these changes
Feb 3, 2023
kuhe
reviewed
Feb 3, 2023
packages/middleware-sdk-transcribe-streaming/src/websocket-handler.ts
Outdated
Show resolved
Hide resolved
3 tasks
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
Fixes #3922
Description
Closes sockets on WebSocketHandler.destroy()
Testing
CI
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.