tokio icon indicating copy to clipboard operation
tokio copied to clipboard

task: add custom SpawnError

Open ipetkov opened this issue 2 years ago • 6 comments

Define a custom SpawnError to be used by task::Builder instead of using io::Error

Refs #4850

Open question: should we have separate errors for spawn_blocking and spawn_local? Seemed overkill to me so I went with a single type

ipetkov avatar Aug 01 '22 22:08 ipetkov

If the spawn functions have different ways in which they can fail, then I would prefer different error types.

Darksonn avatar Aug 12 '22 08:08 Darksonn

If the spawn functions have different ways in which they can fail, then I would prefer different error types.

I'm pretty busy for the next week but I will try adding different error types afterwards and see how it works out

ipetkov avatar Aug 17 '22 19:08 ipetkov

Errors have been split out into:

  • SpawnError for "regular" spawn
  • SpawnBlockingError for spawn_blocking
  • SpawnLocalError for spawn_local

This should give us the ability to evolve the different errors independently of each other

ipetkov avatar Aug 30 '22 01:08 ipetkov

Looking through this and thinking a bit more about it (now that I also have all the runtime context loaded), I would probably not do it this way. I would try to avoid touching the schedule trait as that is a very hot path. A return value there with an error (depending on the struct size) could end up being measurable.

Instead, I would probably add try_spawn for each scheduler and implement it by doing something like (pseudocode):

fn try_spawn(&self, task: T) -> SpawnResult<JoinHandle> {
    if self.is_closed() {
        return Err(...);
    }

    Ok(self.spawn(task))
}

This is racy for the multi-threaded scheduler, but there is no way to make it not racy.

If the goal is to land a SpawnError as a first step, I would do a PR with just SpawnError and replace io::Resuilt w/ SpawnResult. Then, in follow-up PRs, add more checks that catch error cases.

carllerche avatar Sep 15 '22 01:09 carllerche

It looks like the plan is to have one error type per spawn method (spawn, spawn_local, spawn_blocking). I think that is reasonable. I don't love having top-level modules polluted with many error types. It makes docs harder to navigate.

One strategy is to make an error module, e.g. tokio::task::error and dump all the error type sin there. The channel types do this: https://docs.rs/tokio/latest/tokio/sync/mpsc/error/index.html

carllerche avatar Sep 15 '22 01:09 carllerche

Also, it looks like JoinError is already exported in tokio::task. What I would probably do is define JoinError in tokio::task::error and then re-export it in tokio::task. We could also maybe doc(hide) the re-export.

carllerche avatar Sep 15 '22 01:09 carllerche

I ran out of bandwidth to work on this 😞

Going to close this as I was planning on starting over in a more targeted manner (plus it's fallen behind from the latest changes). Hopefully I'll find time to come back to this next year!

ipetkov avatar Dec 29 '22 01:12 ipetkov