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.

Sign up for GitHub

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

Jump to bottom

trace-core: When dispatching an event, the dispatcher should unset itself #1032

Closed
hawkw opened this issue Apr 3, 2019 · 1 comment · Fixed by #1033
Closed

trace-core: When dispatching an event, the dispatcher should unset itself #1032

hawkw opened this issue Apr 3, 2019 · 1 comment · Fixed by #1033
Assignees
@hawkw
Labels
C-bug Category: This is a bug.

Comments

@hawkw
Copy link
Member

hawkw commented Apr 3, 2019

When dispatching an event, the dispatcher should unset itself from the current dispatcher thread-local variable. This will prevent any events in code paths called by the subscriber from creating an infinite loop.

See also tokio-rs/gsoc#1 (comment):

In general, we almost certainly do not want subscribers instrumented w/ tokio-trace to dispatch events to themselves.

@hawkw This brings up a good point. When calling a subscriber, the dispatcher probably wants to remove itself from the thread-local variable.
@hawkw hawkw added C-enhancement Category: A PR with an enhancement or bugfix. tokio-trace labels Apr 3, 2019
@hawkw hawkw self-assigned this Apr 3, 2019
@hawkw hawkw added C-bug Category: This is a bug. and removed C-enhancement Category: A PR with an enhancement or bugfix. labels Apr 3, 2019
@hawkw
Copy link
Member Author

hawkw commented Apr 3, 2019

I have a working implementation of this in a branch, but it causes a bit of a performance hit in Dispatch::get_default. Trying to work around that...

@hawkw hawkw mentioned this issue Apr 4, 2019
Merged
hawkw added a commit that referenced this issue Apr 16, 2019
@hawkw
trace-core: Dispatchers unset themselves (#1033)
4bfa4ff 
This branch changes `dispatcher::get_default` to unset the thread's
current dispatcher while the reference to it is held by the closure.
This prevents infinite loops if the subscriber calls code paths which
emit events or construct spans. 

Note that this also means that nested calls to `get_default` inside of a
`get_default` closure will receive a `None` dispatcher rather than the
"actual" dispatcher. However, it was necessary to unset the default in
`get_default` rather than in dispatch methods such as `Dispatch::enter`,
as when those functions are called, the current state has already been
borrowed.

Before:
```
test enter_span              ... bench:           3 ns/iter (+/- 0)
test span_no_fields          ... bench:          51 ns/iter (+/- 12)
test span_repeatedly         ... bench:       5,073 ns/iter (+/- 1,528)
test span_with_fields        ... bench:          56 ns/iter (+/- 49)
test span_with_fields_record ... bench:         363 ns/iter (+/- 61)
```

After:
```
test enter_span              ... bench:           3 ns/iter (+/- 0)
test span_no_fields          ... bench:          35 ns/iter (+/- 12)
test span_repeatedly         ... bench:       4,165 ns/iter (+/- 298)
test span_with_fields        ... bench:          48 ns/iter (+/- 12)
test span_with_fields_record ... bench:         363 ns/iter (+/- 91)
```

Closes #1032 

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Jun 25, 2019
@hawkw
trace-core: Dispatchers unset themselves (#1033)
802f94a 
This branch changes `dispatcher::get_default` to unset the thread's
current dispatcher while the reference to it is held by the closure.
This prevents infinite loops if the subscriber calls code paths which
emit events or construct spans. 

Note that this also means that nested calls to `get_default` inside of a
`get_default` closure will receive a `None` dispatcher rather than the
"actual" dispatcher. However, it was necessary to unset the default in
`get_default` rather than in dispatch methods such as `Dispatch::enter`,
as when those functions are called, the current state has already been
borrowed.

Before:
```
test enter_span              ... bench:           3 ns/iter (+/- 0)
test span_no_fields          ... bench:          51 ns/iter (+/- 12)
test span_repeatedly         ... bench:       5,073 ns/iter (+/- 1,528)
test span_with_fields        ... bench:          56 ns/iter (+/- 49)
test span_with_fields_record ... bench:         363 ns/iter (+/- 61)
```

After:
```
test enter_span              ... bench:           3 ns/iter (+/- 0)
test span_no_fields          ... bench:          35 ns/iter (+/- 12)
test span_repeatedly         ... bench:       4,165 ns/iter (+/- 298)
test span_with_fields        ... bench:          48 ns/iter (+/- 12)
test span_with_fields_record ... bench:         363 ns/iter (+/- 91)
```

Closes #1032 

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hawkw hawkw

Labels
C-bug Category: This is a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

trace-core: Dispatchers unset themselves tokio-rs/tokio
1 participant
@hawkw