Skip to content

Bugfix: process messages after websocket closes #123

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Bugfix: process messages after websocket closes #123

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion deepgram/transcription.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from enum import Enum
from warnings import warn
import websockets.client
import websockets.exceptions
from ._types import (Options, PrerecordedOptions, LiveOptions, ToggleConfigOptions,
TranscriptionSource, PrerecordedTranscriptionResponse,
LiveTranscriptionResponse, Metadata, EventHandler)
Expand Down Expand Up @@ -239,7 +240,8 @@ async def _receiver(self) -> None:
try:
body = await self._socket.recv()
self._queue.put_nowait((True, body))
except Exception as exc:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this still catch Exception after the new exception catch just in case some other error happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think we should not catch all exceptions. My guess is that the original code caught all exceptions specifically so that the ConnectionClosedOk exception was handled correctly. Below is more info on my opinion:

This PR also makes a small change to the exception handling. Rather than catching all websocket closures (and other any exceptions!) by using except Exception as exc, I replaced the error handling with a more explicit catch: except websockets.exceptions.ConnectionClosedOK:. This change will result in errors that are not ConnectionClosedOK bubbling up to the user. I think this is appropriate because any other websocket closures are errors that the user may want to handle or be aware of. I was unable to produce any situations where a different exception was thrown, but we may get messages from customers who find edge cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we catch those other errors and log them rather than throwing them instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamienDeepgram what makes you want to catch and log these specific errors rather than throwing them?

The new try/except clauses catches the 1011 "error" and closes the websocket cleaning when that response code is received. Deepgram returns the 1011 "error" as a notification to the user that Deepgram closed the socket due to inactivity, which the previous version of the Python SDK considered a normal piece of the workflow (i.e. not an exceptional case).

The new code handles the 1011 "error" in the same way, but raises other errors to the user. I think that makes sense because those other errors are real errors (whereas the 1011 error is a simply closure due to inactivity). But I'm curious to hear what you're thinking.

except websockets.exceptions.ConnectionClosedOK:
await self._queue.join()
self.done = True # socket closed, will terminate on next loop

def _ping_handlers(self, event_type: LiveTranscriptionEvent,
Expand Down