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

fix: handle socket connection closed error in _signal_exit #355

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

roerohan
Copy link
Contributor

@roerohan roerohan commented Apr 5, 2024

Proposed changes

In the _listening function, when there's a WebSocketException, the following line runs as a part of the error handling procedure.

Now, the WebSocketException might also be caused due to the socket connection breaking. In this case, an error is thrown of the following form

future: <Task finished name='Task-291' coro=<AsyncLiveClient._listening() done, defined at /usr/local/lib/python3.12/dist-packages/deepgram/clients/live/v1/async_client.py:171> exception=ConnectionClosedError(Close(code=1011, reason='Deepgram did not receive audio data or a text message within the timeout window. See https://dpgr.am/net0001'), Close(code=1011, reason='Deepgram did not receive audio data or a text message within the timeout window. See https://dpgr.am/net0001'), True)>

While handling this exception, the _signal_exit function is called, which has the following lines

async def _signal_exit(self) -> None:
# send close event
self.logger.verbose("closing socket...")
if self._socket is not None:
self.logger.verbose("send CloseStream...")
await self._socket.send(json.dumps({"type": "CloseStream"}))
await asyncio.sleep(0.5)
# push close event
await self._emit(
LiveTranscriptionEvents.Close,
CloseResponse(type=LiveTranscriptionEvents.Close.value),
)

In line 488, we're doing a self._socket.send, which is bound to fail because the socket connection is closed. This throws an error from the _signal_exit method. Since _signal_exit is not wrapped in a try except, the _listening method also throws an error and exits. The following is a sample traceback.

Traceback (most recent call last):
  File "/usr/local/lib/python3.12/dist-packages/deepgram/clients/live/v1/async_client.py", line 287, in _listening
    await self._signal_exit()
  File "/usr/local/lib/python3.12/dist-packages/deepgram/clients/live/v1/async_client.py", line 478, in _signal_exit
    await self._socket.send(json.dumps({"type": "CloseStream"}))
  File "/usr/local/lib/python3.12/dist-packages/websockets/legacy/protocol.py", line 635, in send
    await self.ensure_open()
  File "/usr/local/lib/python3.12/dist-packages/websockets/legacy/protocol.py", line 939, in ensure_open
    raise self.connection_closed_exc()

In this PR, I've added a try except only around the self._socket.send. It might be necessary to add exception handling wherever _signal_exit is being called as well, as when _signal_exit throws an error, the while True loop will exit, and the listen_thread will not exist anymore. Therefore any transcripts will not be received for that connection.

Closes #356

Types of changes

What types of changes does your code introduce to the community Python SDK?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update or tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • I have lint'ed all of my code using repo standards
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@davidvonthenen
Copy link
Contributor

Ooooh very nice catch!

This needs to also be done for the sync client here, if you can add that would be great:
https://github.com/deepgram/deepgram-python-sdk/blob/main/deepgram/clients/live/v1/client.py#L476

If you could replace this exception handling with this to provide more detail on the error, that would be great as well:

                except websockets.exceptions.ConnectionClosedOK as e:
                    self.logger.notice(f"_signal_exit  - connection closed: {e.code}")
                except websockets.exceptions.WebSocketException as e:
                    self.logger.error("_signal_exit - WebSocketException: {str(e)}")
                except Exception as e:
                    self.logger.error("_signal_exit - Exception: {str(e)}")

Thanks for finding this issue!

@roerohan
Copy link
Contributor Author

roerohan commented Apr 5, 2024

@dvonthenen thanks for the swift response!

I've updated the PR, please take a look and let me know if I should make any other changes too.

@davidvonthenen
Copy link
Contributor

This looks great! I appreciate finding and fixing this issue!

@davidvonthenen davidvonthenen merged commit 5dee108 into deepgram:main Apr 5, 2024
1 check passed
@davidvonthenen davidvonthenen mentioned this pull request Apr 5, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhandled exceptions in AsyncLiveClient._listening
2 participants