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.
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
Fix healthcheck and streaming with whisper #59
base: next
Are you sure you want to change the base?
Fix healthcheck and streaming with whisper #59
Changes from 9 commits
4341d9e
f66a3ac
c153a34
1af95a2
84edaf7
6813545
754974d
5e44b2c
d04c13a
01a5297
ccfc951
35ef389
cbec83d
8b1c412
b97c661
f4f3971
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping_interval / ping timeout : is that related to TCP breaks (HTTP WS keepAlive) ? Check comment below on handling only ConnectionClosedOK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is to avoid closing websocket when it is not wanted. When testing, if I did not send any messages for a long time, the websocket closed by itself. By disabling the ping it doesn't close anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, only ConnectionClosedOK is handled to cancel the websocket. Please review whether other exceptions could occur in this context. For example, a TCP RST might trigger a ConnectionClosed event, similar to how WebSocketDisconnect is handled in FastAPI.
Additionally, a finally block might be missing to handle unexpected exceptions and actively close the context for the current socket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of that script, that is supposed to be just for testing the streaming locally I'm not sure if we can encounter other errors. I will add a finally to close the websocket and try to see if something else can be triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok,
Check actual implementation in the real Websocket server to ensure corner-cases are well handled please.