-
Notifications
You must be signed in to change notification settings - Fork 54
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
Handle errors from AudioRenderThread::start #424
Conversation
Signed-off-by: Taym <haddadi.taym@gmail.com>
@jdm FYI |
Oops, I should have mentioned that there was someone else already looking into servo/servo#32986. |
8a9e636
to
b9b4670
Compare
Signed-off-by: Taym <haddadi.taym@gmail.com>
b9b4670
to
aa2dd33
Compare
Since this PR exists, you should keep going with it. Thanks for looking into this! |
Signed-off-by: Taym <haddadi.taym@gmail.com>
Signed-off-by: Taym <haddadi.taym@gmail.com>
audio/context.rs
Outdated
Self { | ||
.expect("Failed to spawn AudioRenderThread"); | ||
|
||
let thread_result: Result<(), AudioSinkError> = result_receiver.recv().unwrap(); |
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.
@jdm I think this is blocking now causing the timeout in other test, I am checking it
Signed-off-by: Taym <haddadi.taym@gmail.com>
audio/context.rs
Outdated
@@ -156,11 +156,14 @@ impl AudioContext { | |||
}) | |||
.expect("Failed to spawn AudioRenderThread"); | |||
|
|||
let thread_result: Result<(), AudioSinkError> = result_receiver.recv().unwrap(); | |||
let thread_result = result_receiver.try_recv(); |
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.
This won't work quite right. The recv call is necessary because we need to block until we receive either a success or fail signal on the channel; if we're timing out in tests, it means that we need to find a way to split the work of setting up the audio backend so that we can signal a success before proceeding with running it synchronously.
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.
okey got it, I will revert last commit and try to change how we do the setup.
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.
@jdm should we merge this one?
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.
I don't think we can merge this without fixing the timeout issue you found, and I think the fix needs to be in this repository.
This reverts commit 5c16fdf. Signed-off-by: Taym <haddadi.taym@gmail.com>
044d15e
to
ade1a2a
Compare
…imeouts. Signed-off-by: Taym <haddadi.taym@gmail.com>
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.
Nicely done!
Part of servo/servo#32986, in order to handle errors in Servo, we need to remove panics and return errors.