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

Improved error handling in the voice client. #210

Merged
merged 11 commits into from
Dec 11, 2023
Merged

Conversation

mdwelsh
Copy link
Contributor

@mdwelsh mdwelsh commented Dec 10, 2023

This PR makes a few minor changes to improve error handling and debugging.

  • We explicitly track the initializing state of the VoiceSession as it goes from created -> connecting -> connected -> audio enabled -> started. This is going to make it easier for applications to observe the state of the session and take appropriate action when errors or timeouts occur.
  • The onError handler carries a VoiceSessionError object with information about the error. This is a simple message for now, but can be expanded later.
  • Added a few additional places where .onError is called.

The more substantive (perhaps) change is that .warmup is no longer called in the ctor or when the socket closes. I would much rather this be done explicitly by application code, rather than the VoiceSession trying to be too clever. Constructors are not a good place to be doing actions like opening socket connections, anyway, because it's much harder to deal with errors that happen during the ctor. By having .warmup() be called explicitly, we can create the VoiceSession, set up the error handling logic, call .warmup, and deal gracefully with connection errors that might occur.

These changes allow us to have more graceful error handling in the Santa app.

packages/fixie/src/voice.ts Show resolved Hide resolved
packages/fixie/src/voice.ts Outdated Show resolved Hide resolved
packages/fixie/src/voice.ts Outdated Show resolved Hide resolved
@mdwelsh
Copy link
Contributor Author

mdwelsh commented Dec 10, 2023

Thanks! PTAL

packages/fixie/src/voice.ts Outdated Show resolved Hide resolved
packages/fixie/src/voice.ts Outdated Show resolved Hide resolved
packages/fixie/src/voice.ts Outdated Show resolved Hide resolved
@mdwelsh mdwelsh requested a review from juberti December 11, 2023 22:39
Copy link
Contributor

@juberti juberti left a comment

Choose a reason for hiding this comment

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

LG, noted two very minor nits

packages/fixie/src/voice.ts Outdated Show resolved Hide resolved
packages/fixie/src/voice.ts Show resolved Hide resolved
@mdwelsh mdwelsh enabled auto-merge (squash) December 11, 2023 23:08
@mdwelsh mdwelsh merged commit ffd0283 into main Dec 11, 2023
2 checks passed
@mdwelsh mdwelsh deleted the mdw/better-error-handling branch December 11, 2023 23:41
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.

2 participants