sync: avoid creating mutex and rwlock resource_span with parent, use None parent instead

[xuorig]

Most of the context for this PR can be found in this issue.

The TRACE resource_span that Mutex and RWLock create when using new can keep open their parent for as long as the mutex lives. This pull request changes these spans to have a parent of None rather than having a parent-child relation ship with the current span.

As mentioned by @davidbarsky in the linked issue, another possibility could be to alter the API and accept a parent span. I'm open to changing this PR although I agree it feels strange when it comes to tracing.

Looking for existing tests, I wasn't sure if regression testing this (which would require bringing a tracing subscriber most likely) was necessary here. Open to adding this as well.

[NobodyXu]

OnceCell, Semaphore etc probably also have the same issue, IMO it's a good idea to also change that in this PR

[Darksonn]

Yeah, if possible I'd like to fix all instances of this.

I don't think we have many tests for the tracing stuff.

Open to adding this as well.

If you want to add tests, then those are always welcome.

[davidbarsky]

I agree with Alice and Jiahao–can you make changes to tokio::sync:OnceCell, tokio::sync::Semaphore, and tokio::sync::Barrier similar to my suggestions on tokio::sync::RwLock/tokio::sync::Mutex ?

[davidbarsky]

@xuorig On the subject of tests, I know that @hds was looking at adding tests for resources in Tokio using tracing-mock. What are your timelines for needing this to land? Would y'all be able to take a dependency on an unmerged PR to Tokio for a few days or nah?

[xuorig]

@davidbarsky absolutely, this can wait. I've added a mock currently but happy to swap it out later 👍

[xuorig]

I just committed a test using tracing-mock before realizing this PR is introducing it more generally. Will wait for it to be merged and modify this PR.

[Darksonn]

The other PR is merged now.

[hds]

After some prototyping with tokio-console (the console-subscriber specifically) and discussions with @hawkw and @davidbarsky, I believe we should not add follows_from relationship to the resource span (and for the same reasons, I don't plan to add it to the task span either).

TLDR: I propose that this change should only add parent: None to these resource spans and make no other changes.

The reason for this is a bit long, but I felt it made sense to write it out so that the justification is available when people come looking at this change.

The reason is that using follows_from with the current span is a bit delicate. You're using the global current span, but specific tracing-subscriber Layers may not be recording that span (it could be filtered out) and this results in the layer never receiving a call to on_follows_from.

Here is an example (I'm illustrating based on task spans, but the same is the case for the sleep resource):

async fn spawn_parent() {
    tokio::task::Builder::new()
        .name("parent_ends")
        .spawn(parent_spawns_child())
        .unwrap();
}

#[tracing::instrument]
async fn parent_spawns_child() {
    tokio::task::Builder::new()
        .name("child_continues")
        .spawn(async {
            loop {
                tokio::time::sleep(Duration::from_secs(100)).await;
            }
        })
        .unwrap();

    tokio::time::sleep(Duration::from_secs(5)).await;
}

Because of the #[tracing::instrument] on parent_spawns_child(), the span hierarchy looks like this:

- runtime.spawn[task.name=parent_ends] (id=1)
  - parent_spawns_child (id=274877906945)
    - runtime.spawn[task.name=child_continues] (id=274877906946)

And that parent_spans_child span in the middle means that a Layer which is only looking for tokio instrumentation (i.e. console-subscriber) will never receive a call to on_follow_from. If I take the #[tracing::instrument] out, then it will.

This means that the console-subscriber (or any subscriber layer) won't be able to rely on follows_from unless it registers for all spans which are active. On the other side, the behavior which we proposed here (providing a "weak" connection to the context within which a resource or task is created) can be safely done within the subscriber layer itself, as the layer has the current span stack from its point of view (containing only spans that it is aware of) and in the example above, the console-subscriber would not see the parent_spans_child span and could itself keep a connection between the two spans that won't cause the parent span to remain open longer than it should.

[xuorig]

PR updated:

• Using parent None and no follows_from relation ship as suggest by @hds.
• Since batch_semaphore is only used internally and also since it does seem to have a parent-child relationship with its callers, I've omitted parent: None from that resource span. Let me know if that makes sense to you all.
• Tests added for all new functions of the sync primitives in sync.

[davidbarsky]

lgtm, but i'll let @hds confirm that it's correct.

[hds]

Looks good to me! Thanks for your work on this change!

I will just note here that the tests (which are great and something very valuable to add), don't test the change, which is to say they're not testing that the resource spans are being created as explicit roots (parent: None). However, there's actually a defect in tracing-mock which means that you can't test for that right now anyway (tokio-rs/tracing#2440).

Once that defect gets fixed, these tests can be updated, but for now we have significantly more test coverage than before, so let's go ahead and merge this change!

[xuorig]

@hds one question about this, just for my own learnings:

don't test the change, which is to say they're not testing that the resource spans are being created as explicit roots

I was under the impression they did:

.new_span(mutex_span.clone().with_explicit_parent(None))

Now I understand there's a defect with the above, but are you saying this wasn't the way to do it in the first place?

[hds]

@xuorig Your expectations are correct. But they're not actually testing what you want them to because there is no contextual parent span when the resource gets created.

To complete the test, we would need to do something like the following (for the Barrier test):

#[tokio::test]
async fn test_barrier_creates_span() {
    let barrier_span = expect::span()
        .named("runtime.resource")
        .with_target("tokio::sync::barrier");

    let size_event = expect::event()
        .with_target("runtime::resource::state_update")
        .with_fields(expect::field("size").with_value(&1u64));

    let arrived_event = expect::event()
        .with_target("runtime::resource::state_update")
        .with_fields(expect::field("arrived").with_value(&0));

    let contextual_parent = expect::span().named("contextual parent");

    let (subscriber, handle) = subscriber::mock()
        // We need to expect our contextual parent span is created and entered
        .new_span(contextual_parent.clone())
        .enter(contextual_parent)
        .new_span(barrier_span.clone().with_explicit_parent(None))
        .enter(barrier_span.clone())
        .event(size_event)
        .event(arrived_event)
        .exit(barrier_span.clone())
        .drop_span(barrier_span)
        .run_with_handle();

    {
        let _guard = tracing::subscriber::set_default(subscriber);
        {
            // We enter into a span so that the barrier will have a contextual parent.
            let _span = tracing::info_span!("contextual parent").entered();
            let _barrier = sync::Barrier::new(1);
        }
    }

    handle.assert_finished();
}

If you run this test on current master it will pass. But if you run it it without the changes in this PR, it will also pass. That's the defect (along with another thing) in tracing-mock that needs to be fixed.