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

Tokio swallows panics #1311

Closed
ry opened this Issue Dec 11, 2018 · 3 comments

Comments

2 participants
@ry
Copy link
Collaborator

commented Dec 11, 2018

Here is a demonstration of Tokio swallowing panics: https://gist.github.com/ry/01ed1055964fe5cba8c38839635a5b2d

Apply that patch (after #1310 is landed) and run deno.readFile("/")... it should crash but does not.

@F001

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

Is blocking function in ops.rs the only way to spawn a task to thread pool?
If that is the case, I'd suggest to catch_unwind by ourselves.

// wrap the closure running in thread pool
fn abort_when_panic<F, R>(f: F) -> R where F: UnwindSafe + FnOnce() -> R
{
  match std::panic::catch_unwind(f) {
      Err(e) => {
        eprintln!("{:?}", e);
        std::process::abort();
      },
      Ok(r) => r,
  }
}

fn blocking<F>(is_sync: bool, f: F) -> Box<Op>
where
  F: 'static + Send + UnwindSafe + FnOnce() -> DenoResult<Buf>,
{
  if is_sync {
    Box::new(futures::future::result(f()))
  } else {
    Box::new(tokio_util::poll_fn(move || convert_blocking(|| abort_when_panic(f) )))
  }
}
@ry

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 12, 2018

No - it's not the only way to spawn a task. Any callback on a future will be run in the thread pool. For example, this callback is executed in the thread pool

deno/src/ops.rs

Lines 666 to 684 in a8c3b44

.and_then(move |(_resource, _buf, nread)| {
let builder = &mut FlatBufferBuilder::new();
let inner = msg::ReadRes::create(
builder,
&msg::ReadResArgs {
nread: nread as u32,
eof: nread == 0,
},
);
Ok(serialize_response(
cmd_id,
builder,
msg::BaseArgs {
inner: Some(inner.as_union_value()),
inner_type: msg::Any::ReadRes,
..Default::default()
},
))
});

@F001

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

Related issue in tokio: tokio-rs/tokio#700

ry added a commit to ry/deno that referenced this issue Apr 14, 2019

Fix silent error and add custom panic handler again
This is to work around Tokio's panic recovery feautre.
Ref tokio-rs/tokio#495
Ref tokio-rs/tokio#209
Ref denoland#1311
Fixes denoland#2097

ry added a commit to ry/deno that referenced this issue Apr 14, 2019

Fix silent error, add custom panic handler
This is to work around Tokio's panic recovery feature.
Ref tokio-rs/tokio#495
Ref tokio-rs/tokio#209
Ref denoland#1311
Fixes denoland#2097

ry added a commit to ry/deno that referenced this issue Apr 14, 2019

Fix silent error, add custom panic handler
This is to work around Tokio's panic recovery feature.
Ref tokio-rs/tokio#495
Ref tokio-rs/tokio#209
Ref denoland#1311
Fixes denoland#2097

ry added a commit that referenced this issue Apr 14, 2019

Fix silent error, add custom panic handler (#2098)
This is to work around Tokio's panic recovery feature.
Ref tokio-rs/tokio#495
Ref tokio-rs/tokio#209
Ref #1311
Fixes #2097

@ry ry closed this in #2188 Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.