rt: add a method to retrieve task id

[agayev]

Fixes #1996.
Closes #4721.

[agayev]

After the first PR, I realized that I haven't put added the test that forces a panic, and after I added that test, it failed; trying to figure out why....

[Darksonn]

In general, this change looks very good. It is rather compact, which is nice. I have a bunch of comments related to making sure that ids are set properly during destructors and that nested tasks handle ids correctly, and I expect some of the requested tests to fail under the current code. However, I believe that the changes to fix this are relatively small.

[agayev]

@hawkw @Darksonn taking a step back and looking at it again, I think it will be cleaner if we store task_id in Stage enum, as in:

pub(super) enum Stage<T: Future> {
    Running(T, Id),
    Finished(super::Result<T::Output>, Id),
    Consumed,
}

I already suggested this to @Darksonn in discord and she dismissed it, but I didn't give my reasoning back then, so let me give it now.

Only three functions need to set the task id in the context:

• CoreStage::poll -- for making id available during the normal execution of the task
• CoreStage::drop_future_or_output -- for making id available when dropping a future or output
• CoreStage::store_output -- for making id available during the dropping of future

If we store id in the Stage enum, we can keep id setting stuff completely within core.rs file in just those functions and not leak it outside, and we can be sure that all future uses of these functions and moving them around will be covered. Otherwise, what we have currently is not reliable because we should find places where one of those functions are called and put TaskIdGuard around it.

The alternative is to change those three functions to accept the id argument and then have them enter the guards. I've partially done this for store_output in this PR, but it is incomplete/inconsistent at the moment: I set TaskGuardId in poll_future, which covers both, the call to core.poll(cx) and the call to store_output. So when the future destructor runs during the call to store_output, it can access the id. I've extended Stage enum to store Id with Finished variant, so store_output saves the Id there, so that when drop_future_output drops the output there, the output destructor can also access the id. As you can tell, this is messy and inconsistent.

Which one do you prefer, or do you have a better suggestion?

[Darksonn]

Thank you, that motivation seems reasonable enough. However, there's also another option: move the CoreStage methods to Core instead, so that they can access the id without changing the struct layout. What do you think?

I would prefer a solution where the id is not deleted from the task when the output is consumed so that the id field can be removed from JoinHandle in the future:

pub struct JoinHandle<T> {
    raw: Option<RawTask>,
    id: Id,
    _p: PhantomData<T>,
}

[agayev]

Thank you, that motivation seems reasonable enough. However, there's also another option: move the CoreStage methods to Core instead, so that they can access the id without changing the struct layout. What do you think?

I would prefer a solution where the id is not deleted from the task when the output is consumed so that the id field can be removed from JoinHandle in the future:

pub struct JoinHandle<T> {
    raw: Option<RawTask>,
    id: Id,
    _p: PhantomData<T>,
}

Sure, that also works. See #5182.

[agayev]

@Darksonn @hawkw rebased and rewrote now that CoreStage functions are in core.

Two points:

• I think I was wrong in my original assessment that store_output also needs guards -- from my cursory reading it appears that by the time store_output is called, the future in the stage is already destroyed by drop_future_or_output.
• Haven't implemented the following test yet from @Darksonn 's first list "If you use block_in_place + spawn + block_on on a current-thread runtime to execute a spawned task within another task, the original task's id is not erased." I'll bug @Darksonn on discord to get more clarity on that test

[Darksonn]

I haven't looked over the tests yet, but in general this looks quite nice and clean. 👍

[hawkw]

overall, the implementation looks good to me! most of my remaining suggestions concern the documentation and tests added in this PR.

[hawkw]

it looks like there are currently some test failures when building with --no-default-features: https://github.com/tokio-rs/tokio/actions/runs/3439239014/jobs/5736297357#step:9:14

[agayev]

@hawkw addressed all the issues you raised. I experimented with github's web interface for committing for the first time and messed up some things, so had to squash, rebase, and force-push -- apologies @Darksonn.

I can't reproduce the build failure even if I try --not-default-features... Let's see if it happens again.

[agayev]

@Darksonn @hawkw passed all the tests -- ready for review!

[Darksonn]

Panics on spawned tasks do not ensure that the test fails. You have to ensure that the test fails if the panic happens. I commented on two of them, but it applies to many more tests.

[Darksonn]

Looks good to me.