-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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: panicking when can't create runtime for block_on #2905
Conversation
aeee807
to
6d03dc7
Compare
6d03dc7
to
763e4f3
Compare
@ry after thorough reading of #2243 I can tell that the issue describes several different problems.
I'd prefer to addresses second point in a separate PR as I need to think a bit more on how to test that properly. I'm not sure there's a reliable reproduction to test issue resolved in this PR, we were just to eager creating runtimes ¯_(ツ)_/¯ |
Actually that might not be true, adding this test on // cli/tokio_util.rs
#[cfg(test)]
mod tests {
use super::*;
use futures;
#[test]
#[allow(unreachable_code)]
#[should_panic]
fn test_block_on() {
let fut = futures::future::lazy(|| {
panic!("unexpected panic during block on");
if false {
return futures::future::err(());
}
futures::future::ok(1)
});
let r = block_on(fut);
assert!(r.is_err());
}
} So I guess this should fix #2243 after all. There are of course other panics (most of them from |
This patch seems fine, but I'm not sure what it's fixing. #2243 seems to be fixed already.... |
@ry this panic: #2243 (comment) |
@bartlomieju are you able to reproduce that panic on v0.17 ? I can’t. |
@ry:
|
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.
Ok cool. thanks!
LGTM
Just one small change - I replaced unwraps on one-shot channel with expects. If those calls fail then there are really too few available files to properly run compiler. |
ed8c9e3
to
4e43e46
Compare
No description provided.