watched_tasks: maintain a list of spawned async tasks and propagate errors

[hnez]

This PR is based on an idea from a discussion in PR #43, where it was noticed that errors from tasks and threads propagate nowhere.

This PR fixed that by assembling a list of threads and tasks that should not complete before the end of the program and propagating all errors from these threads and tasks to main() and beyond.

This touches a lot of places, as we spawn quite a lot of async_std tasks (the tacd logs now say "Spawned 58 tasks tasks and 5 threads").

This PR allows us to add better error handling for tasks and threads (we can for example use the ? operator to propagate errors out and do not have to resort to .unwrap() to at least crash the tacd from within a task on error). Adding this better error handling is out of scope for this PR, as it is already large enough.

[hnez]

The failed cargo deny run should be fixed by the dependency updates in #42 and has nothing to do with this PR.

[KarlK90]

Nice! I like the changes and implementation. I thought whether or not the async runtime provides mechanisms to query running tasks and see if anything failed like tokio-console, but this might be messy (or not even possible). Anyway, there are some minor things that should be corrected but apart from that LGTM.

[hnez]

While addressing your comments I have also changed the way ThreadHandle notifies a task about the completion of a thread.
Using a async_std::channel there was somewhat of a hack so I do not have to work with Future and Waker directly, because that sounded somewhat scary.
As it turns out working with Wakers is not scary at all, so we can just use one here:

tacd/src/watched_tasks.rs

Lines 29 to 54 in 69d52f7

	impl ThreadHandle {
	fn new<F>(name: String, function: F) -> Result<Self>
	where
	F: FnOnce() -> TaskResult + Send + 'static,
	{
	let wake_on_exit = Arc::new(Mutex::new(None::<Waker>));
	let wake_on_exit_thread = wake_on_exit.clone();
	let handle = thread::Builder::new().name(name).spawn(move || {
	// We could std::panic::catch_unwind() here in the future to handle
	// panics inside of spawned threads.
	let res = function();
	// Keep the Mutex locked until exiting the thread to prevent
	// a race with another thread checking for task completion and
	// adding a waker.
	let mut wake_on_exit = wake_on_exit_thread
	.lock()
	.map_err(|_| anyhow!("Tried to lock a tainted Mutex"))?;
	if let Some(waker) = wake_on_exit.take() {
	waker.wake();
	}
	res
	})?;

This does however mean that you have to re-review that part.

[hnez]

The failed cargo clippy run should be fixed by #49 and has nothing to do with this PR.

[KarlK90]

This looks good to me now and will definitely improve the error handling story in the code base. Feel free to merge.

[jluebbe]

Adding a custom wrapper around standard APIs seems very invasive to me. Is this really such an unusual problem that there is no idiomatic way?

[hnez]

Hi @jluebbe

no idiomatic way

Not in async_std itself, so we would have to use a crate for it. I had a look around and found async_nursery which may be coerced into doing what we we want (minus the thread handling), but I would prefer not to use it because it does not look too mature (and does not support threads in addition to tasks).

I quite like the tailor-made solution implemented in this PR and think 206 not-that-dense lines of code are an acceptable complexity compared to adding a new dependency to handle this for us.

I would prefer merging this as-is rather than adding a new dependency.

[hnez]

The recently merged pull requests #33 and #34 made some changes to this PR necessary, as both of them spawn tasks.
Please re-review affected files.

[hnez]

I've rebased this PR on top of recent main and think it should be the next one we merge.

[KarlK90]

I remember that @jluebbe wanted some written commentary why we choose to implement a bespoke solution and not use async_nursery or tokio's JoinSet. Could you add this to watched_tasks.rs? After that I'm happy to approve this.

[hnez]

I've added a comment …

tacd/src/watched_tasks.rs

Lines 11 to 29 in 6c7a2cd

	// This is a wrapper around async_std::task:spawn() that keeps track of the
	// tasks it spawned. This solves the problem of error propagation from tasks
	// for us.
	// When using async_std::task:spawn() you get a handle back that can be used
	// to check if and how the task has completed, but there is no common way in
	// async_std to have a list of tasks to watch at the same time.
	// We want to keep track of the various long running tasks we spawn in the
	// tacd and want to propagate errors from back to main().
	// We also want the same, with a similar API for actual threads instead of
	// async tasks. WatchedTasks does both.
	//
	// There are other solutions that do basically the same, but did not quite
	// fit our needs:
	//
	// - async_nursery - Works for async_std but does not handle actual threads
	// and does not look that great code-wise.
	// - tokio JoinSet - Does roughly the same as WatchedTasks, but also without
	// native thread support and - more importantly for tokio (which we do
	// not use).

… that talks about why we went with our own task spawning wrapper instead of using something off-the-shelf.

[KarlK90]

@hnez The comment summarizes the discussion very well. You have my approval - feel free to merge.