rt: do not trace tasks while locking OwnedTasks

[jswrenn]

If a polled tasks completes while OwnedTasks is locked, it will not be able to remove itself from OwnedTasks, resulting in a deadlock.

Fixes #6035.

[Darksonn]

We should be taking a refcount to the task instead of using as_raw.

[jswrenn]

What's the API for that?

[Darksonn]

You can add this:

impl<S: 'static> Clone for Task<S> {
    fn clone(&self) -> Task<S> {
        self.raw.ref_inc();
        Task::new(self.raw)
    }
}

[jswrenn]

Done!

[Darksonn]

Huh. I realized that transition_to_notified_for_tracing actually performs a refcount increment. Because of that, you don't need the additional refcount increment. However, I would probably prefer to see this refcount management a bit more wrapped up in a less error-prone api.

[jswrenn]

Hm. Unfortunately, now I'm quite confused by how refcount management is supposed to work. I removed only the increment in transition_to_notified_for_tracing, but doing so causes the tests to SIGABRT:

free(): invalid pointer

What's the relationship between notifying a task, polling a task, and its refcount?

[Darksonn]

The behavior of each function is documented in harness.rs. There's also a bunch of documentation at the top of mod.rs.

Basically, each Task (or Notified) object owns a refcount. Various methods will consume the refcount. We want the refcount that transition_to_notified_for_tracing creates to be owned by a Task somehow, rather than implicitly via a RawTask. This is because it generally makes code much more clear, and it also makes the code more robust towards error paths due to RAII cleanup.

[jswrenn]

Got it, thanks! So, to summarize, the crash occurred because:

• cloning Task incremented the refcount
• polling the task decremented the refcount
• dropping Task decremented the refcount

...which is one too many decrements. transition_to_notified_for_tracing needs to increment the refcount (much like transition_to_notified_by_ref), because polling is going to consume that count.

[Darksonn]

LGTM.

Edit: I confused myself...

[Darksonn]

Okay, so, I've figured out how I think we should handle this refcount business:

• First, we wrap transition_to_notified_for_tracing with a method on Task that returns a Notified. This Notified will own the refcount that we created inside of transition_to_notified_for_tracing.
• Next, when you want to poll the task, instead of calling poll directly, you instead use OwnedTasks::assert_owner to convert the Notified into a LocalNotified.
• To poll the task, you call LocalNotified::run.

This way, we only increment the refcount once (so there's no call to Task::clone), and all refcounts we create are owned by Notified object, so their destructor will clear it if it gets dropped before it is polled, e.g. due to a panic.

[jswrenn]

Done!

[Darksonn]

Since transition_to_notified_for_tracing already increments the refcount, you do not also need to clone it. You probably have a memory leak right now.

Suggested change

	Notified(self.clone())
	Notified(self)

[jswrenn]

I don't think there's a leak here. Remember, when we're calling notify_for_tracing, we're iterating over OwnedTasks, and need to get notified copies of those tasks into a separate vector. A clone has to occur somewhere. Either we can do it here, or we can modified this method to consume self and do the clone in trace_helper instead.

[Darksonn]

As far as I can tell, you increment twice (in transition_to_notified_for_tracing and clone), but only decrement once (in run)?

[jswrenn]

Ah, yep. I didn't catch that run mem::forgets the LocalNotified.

[Darksonn]

Instead of removing the ref_inc from transition_to_notified_for_tracing, it makes more sense to me to remove the call to clone. This way, you only touch the atomic once instead of twice.

[jswrenn]

Done in 4264f61, but note that there's now a need for cloning in trace_owned, because we have to stash the Tasks (or Notified)s outside of OwnedTasks.

[Darksonn]

You should be able to fix that like this:

Suggested change

	pub(super) fn notify_for_tracing(self) -> Notified<S> {
	pub(super) fn notify_for_tracing(&self) -> Notified<S> {

[jswrenn]

That's a compile error. Notified wraps a Task, not a &Task.

error[E0308]: mismatched types
   --> tokio/src/runtime/task/mod.rs:368:22
    |
368 |             Notified(self)
    |             -------- ^^^^ expected `Task<S>`, found `&Task<S>`
    |             |
    |             arguments to this struct are incorrect
    |
    = note: expected struct `runtime::task::Task<S>`
            found reference `&runtime::task::Task<S>`

[Darksonn]

Since you created a new refcount, you can create a new Task using the raw field.

[jswrenn]

Got it, thanks! Done.

[Darksonn]

Just one nit remaining.