Report when a task doesn't / hasn't yielded in a while.

[tobz]

While using tokio-console for debugging some weird behavior -- by the way, tokio-console has been great so far! -- I ended up discovering that my original issue was related to a task that never yielded. While tokio-console helped me figure this out by displaying that a companion task hadn't yet been woken up (because of the other task not yielding), it would have saved me a step or two if instead I was able to see that the non-yielding task hadn't yielded in a while/at all.

Not sure if this is simple to surface or not, but figured I'd file an issue for it regardless. :)

[hawkw]

This should be pretty easy. I think the only difficult part is that we'll have to determine how long a task should have run before yielding before adding a warning.

[hawkw]

@tobz if you're interested in working on this, the way we'd do it is adding a new type implementing the Warn trait:

console/console/src/warnings.rs

Lines 4 to 47 in 7d16ead

	/// A warning for a particular type of monitored entity (e.g. task or resource).
	///
	/// This trait implements the logic for detecting a particular warning, and
	/// generating a warning message describing it. The [`Linter`] type wraps an
	/// instance of this trait to track active instances of the warning.
	pub trait Warn<T>: Debug {
	/// Returns `true` if the warning applies to `val`.
	fn check(&self, val: &T) -> bool;
	/// Formats a description of the warning detected for a *specific* `val`.
	///
	/// This may include dynamically formatted content specific to `val`, such
	/// as the specific numeric value that was over the line for detecting the
	/// warning.
	///
	/// This should be a complete sentence describing the warning. For example,
	/// for the [`SelfWakePercent`] warning, this returns a string like:
	///
	/// > "This task has woken itself for more than 50% of its total wakeups (86%)"
	fn format(&self, val: &T) -> String;
	/// Returns a string summarizing the warning *in general*, suitable for
	/// displaying in a list of all detected warnings.
	///
	/// The list entry will begin with a count of the number of monitored
	/// entities for which the warning was detected. Therefore, this should be a
	/// sentence fragment suitable to follow a count. For example, for the
	/// [`SelfWakePercent`] warning, this method will return a string like
	///
	/// > "tasks have woken themselves more than 50% of the time"
	///
	/// so that the warnings list can read
	///
	/// > "45 tasks have woken themselves more than 50% of the time"
	///
	/// If the warning is configurable (for example, [`SelfWakePercent`] allows
	/// customizing the percentage used to detect the warning), this string may
	/// be formatted dynamically for the *linter*, but not for the individual
	/// instances of the lint that were detected.
	//
	// TODO(eliza): it would be nice if we had separate plural and singular
	// versions of this, like "56 tasks have..." vs "1 task has...".
	fn summary(&self) -> &str;
	}

This would be pretty similar to existing warnings, like SelfWakePercent:

console/console/src/warnings.rs

Lines 118 to 135 in 7d16ead

	impl Warn<Task> for SelfWakePercent {
	fn summary(&self) -> &str {
	self.description.as_str()
	}
	fn check(&self, task: &Task) -> bool {
	let self_wakes = task.self_wake_percent();
	self_wakes > self.min_percent
	}
	fn format(&self, task: &Task) -> String {
	let self_wakes = task.self_wake_percent();
	format!(
	"This task has woken itself for more than {}% of its total wakeups ({}%)",
	self.min_percent, self_wakes
	)
	}
	}

I think that in the check method for this, we'd want to flag a task for the warning if all of the following are true:

1. it is currently active (so the warning doesn't fire for tasks that are waiting to run)
2. it has never yielded
3. it has been running for a period of time >= some threshold (configurable?)

[hawkw]

we probably want to be fairly generous with the threshold, to avoid false positives...it could be something like 5-10 ms? I don't think we want the lint to fire on particularly short-lived tasks; if a task only has two or three await points in it, it's entirely possible that one instance of that task might just be lucky enough that they're all ready, and that's fine if it runs for like, 1ms or less!

[hawkw]

@tobz still planning on working on this?

[tobz]

As interesting as it may be, I think my spare time is a bit too small at the moment to take this. :(

…

On Wed, Jan 12, 2022, 2:04 PM Eliza Weisman ***@***.***> wrote: @tobz <https://github.com/tobz> still planning on working on this? — Reply to this email directly, view it on GitHub <#203 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABWLF6BTMA4HXXECUSPLWDUVXGFDANCNFSM5KERGL6Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[fbrv]

Hi there,
I'm learning Tokio and Rust and I think this issue could be interesting for me, if no one else is working on that I could give it a try

[hawkw]

@fbrv sorry i didn't reply sooner --- are you still interested in working on this issue? If so, please feel free to open a PR, or let me know if you have any questions!

[rybertm]

Hi @hawkw, I'm considering working on this if I can, but have a question, my assumptions may be wrong so forgive me if that's the case.

I think that in the check method for this, we'd want to flag a task for the warning if all of the following are true:
it is currently active (so the warning doesn't fire for tasks that are waiting to run)
it has never yielded
it has been running for a period of time >= some threshold (configurable?)

Does checking if a task is running and for longer than the threshold check all 3?

My assumptions being:

1. It is currently active - Meaning it is not awakened nor completed. Is an idle task considered active?
2. It has never yielded - Meaning it is still running

[jefftt]

Hi! I saw that this went a little stale so I opened up a PR, happy to take feedback on it! Thanks

[yerke]

Since #439 got merged, should this issue be closed?

[Rustin170506]

Yes. I think we can close it now. Thank you!