fix: handle socket connection closed error in _signal_exit

roerohan commented 2 months ago

Proposed changes

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

https://github.com/deepgram/deepgram-python-sdk/blob/d2c43344e723df99468bff161e5e2a1c29600fd8/deepgram/clients/live/v1/async_client.py#L371

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 https://github.com/deepgram/deepgram-python-sdk/blob/d2c43344e723df99468bff161e5e2a1c29600fd8/deepgram/clients/live/v1/async_client.py#L483-L496

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

[x] 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.

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

Further comments

dvonthenen commented 2 months ago

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 commented 2 months ago

@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.

dvonthenen commented 2 months ago

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

deepgram / deepgram-python-sdk