Skip to content
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

feat: detect recursive calls that can deadlock HierarchicalLayer #82

Merged
merged 2 commits into from
May 22, 2024

Conversation

mladedav
Copy link
Contributor

This should address tokio-rs/tracing#2635

If a tracing::event! is created while inside on_event, on_new_span, or on_close, the code can deadlock because of the bufs mutex.

This patch ensures that there are no recursive calls when locking is used by using a thread local atomic boolean which signals whether the call is recursive or not.

This is a very specific feature accommodating usage in rustc so it might be hidden behind a feature flag or some configuration?

@oli-obk
Copy link
Collaborator

oli-obk commented May 22, 2024

lgtm. Tho it could use a test. lmk if you don't have time to add it, I'll give it a look then.

@mladedav
Copy link
Contributor Author

I don't know how to test this. I would either have to make something that's already used log something. I didn't see anything like that when skimming it again. Otherwise, I'd need rustc make the internal write! macros to emit something like they do in the issues, but I assume that cannot be done with the distributed library and compiling stdlib as part of tests here seems a bit too expensive.

Alternatively, I could add a random #[cfg(test)] tracing::error!("Ignore me"); somewhere inside the on_event method, but that didn't seem appropriate either.

So I'd be happy to add tests, but I'm not sure how to go about that here.

@oli-obk
Copy link
Collaborator

oli-obk commented May 22, 2024

Hmm... would it work if we used a custom tracing destination? (i.e. not the stderr default, but a custom writer, that will then just emit a tracing event once)

@mladedav
Copy link
Contributor Author

You're right, that should work, I did not think of that. I'll try it out later today.

@davidbarsky
Copy link
Owner

we really should get tracing-mock out, which would be a great for this.

@mladedav
Copy link
Contributor Author

I've added the test. I know usually tracing tests use with_default instead of things like set_global_default, but in this case that actually circumvents the bug. tracing internally calls get_default which returns NoSubscriber for nested calls if it is inside a scope with custom subscriber. So this had to go into its own integration test to not interfere with other stuff.

@oli-obk
Copy link
Collaborator

oli-obk commented May 22, 2024

Very nice! Thanks!

@oli-obk oli-obk merged commit 16deb7c into davidbarsky:main May 22, 2024
4 checks passed
@mladedav mladedav deleted the dm/recursive branch May 22, 2024 22:13
@mladedav
Copy link
Contributor Author

Could you also make a release when you get some time? It should help the people in the linked tracing issue.

@oli-obk
Copy link
Collaborator

oli-obk commented May 23, 2024

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants