-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
RFC: Task Hook Inheritance #7306
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
Comments
This seems very useful to me. Just earlier today, I was helping somebody figure out how to instrument a task to accomplish very similar goals. Their specific use case was, they had a body they were returning in a hyper-based server handler, that contained a stream to data from a secondary backend service. They wanted to know, what portion of e2e latency came from their backend service (IE the streaming body future), versus a slow client taking a long time to read from the socket, versus scheduling delay or other overhead. So the Presumably the proposed API would also be useful for simplifying The main thing I DON'T see that might be nice, would be an |
A few things came to mind regarding the current impl:
Some changes I think I'd want as a result: If using If using fn with_current_task_hook<F>(f: F)
where
F: FnOnce(&mut dyn Any); |
That's exactly what we want to know too with the service we're building that led to us bothering @Noah-Kennedy :) We currently have an event loop based service, which we are migrating away from, where we measure the timestamp at:
and then calculate a bunch of durations from all that to get answers to the kinds of questions you proposed above.
When we were initially discussing this proposal, the idea was that we would also get, when each hook is invoked, a pointer with a bunch of useful timestamps, such as:
and also potentially on Those timestamps would remove the need for an additional on_wake hook, which from my understanding would often end up being executed as part of io driver work, which is a shared hot path across all the threads and so you'd want to avoid doing that. |
The idea is that you would have something like an
that'd be a great addition as we iterate imo. |
That reminds me, I brought up this hooks overhaul initiative a few months back with my colleages. One suggested that |
@conradludgate interesting, what use cases do you have for this? |
Oh yes, I thought about this a while back too and completely forgot about it! The use case I was thinking of is some fairly basic prioritisation logic, where you could group tasks into priority types and when you're under resource pressure just decide to not schedule a certain category of task at all, so before_poll could say "actually, no, don't wake again for another second", or similar. |
I think that we should consider whether the implementation is compatible with placing the task hook object directly inside the Tokio task, and not behind a |
That's definitely better from a performance standpoint. From an API standpoint, what what an API look like which supports this? |
Also, does this block the merge of the initial PR? I don't think it should. |
Uh oh!
There was an error while loading. Please reload this page.
Problem
Task hooks are the ideal mechanism for users of Tokio to implement their own telemetry. As they allow users to instrument actions such as spawning, termination, and polling, they give users a mechanism to instrument both the lifecycle and running loop of individual tasks.
Unfortunately, the current API does not offer a great way to track state between hooks. We offer a struct which contains an id which could be used as an index into an out-of-band map, but that requires expensive synchronization which is wholly unnecessary - the operations we expose via hooks are already implicitly synchronized by the runtime. More importantly however, it is often required that a tree of tasks be tracked together.
For example, Cloudflare has existing instrumentation for our legacy proxy stack that allows us to track the time spent processing a given request by an event loop, which we would like to carry over to new Rust proxy instances we are spinning up. We can safely assume that the sub-tree of tasks stemming from a top-level task are related, so if we could track state somehow across a tree of tasks in hooks, we could implicitly instrument a whole sub-tree without making invasive modifications to every crate in the tree which spawn Tokio tasks so that we can pass along context information and centrally record which task ids are children of which parents.
Solution
#7197 overhauls task hooks to use a harness trait object which provides implementations of task hooks. The user-implemented type can store whatever state the user wants to store. Propagation of parent-to-child state is handled by a hook which allows parent task harnesses to return a hook harness for a child which is being spawned. So long as default implementations are provided, adding new hooks will not be a breaking change.
Alternatives
I attempted to construct a mechanism to allow users to pass through a set of hooks which shared a generic type for their state to the runtime builder which tokio would box so that we could keep the existing API, but that could not be made to work with hooks passed to new tasks as explicit overrides to any inheritance when spawning, and was a nightmare to deal with. Another alternative which were explored included passing
Box<dyn Any>
to hooks to allow them to store whatever they wanted.Both of those also required adding a hook which would be invoked when a parent task spawned a child so that parent tasks could marshall data via an OOB mechanism the user would come up with which the child task's spawn hook would pull in. The harnesses system accomplish the same thing, works quite similarly under the hood, and is much simpler to use.
The text was updated successfully, but these errors were encountered: