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

Relax restrictions on writer for tracing_appender::non_blocking::NonBlocking #2607

Merged
merged 2 commits into from
Oct 15, 2023
Merged

Relax restrictions on writer for tracing_appender::non_blocking::NonBlocking #2607

merged 2 commits into from
Oct 15, 2023

Conversation

AnthonyMikh
Copy link
Contributor

@AnthonyMikh AnthonyMikh commented Jun 4, 2023 •
edited
Loading

Motivation

NonBlocking from tracing-appender wraps a writer and requires that writer to implement Sync (among other bounds). However it is not clear why this bound is necessary in first place: this writer is sent to a dedicated thread and never used directly concurrently.

#1934 demonstrates that it is a real problem for some people. Also at my current work we hit this issue as well when a third-party writer did not implement Sync. Our workaround was to wrap that writer in our own type and manually implement Send and Sync for it. Needless to say that it is more a hack than a proper solution.

Solution

Remove Sync bound in relevant places. Yep, that simple. Probably having it in first place was just an oversight.

Closes #1934

@AnthonyMikh AnthonyMikh requested a review from a team as a code owner June 4, 2023 17:59
hawkw
hawkw approved these changes Oct 15, 2023
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, this looks good to me --- I don't know why the Sync bound was ever added here, since it's clearly not necessary. Thanks for the fix!

@hawkw hawkw enabled auto-merge (squash) October 15, 2023 18:37
@hawkw hawkw merged commit 3a80127 into tokio-rs:master Oct 15, 2023
55 checks passed
@AnthonyMikh AnthonyMikh deleted the fix/nonblocking-not-sync-writer branch October 15, 2023 19:09
davidbarsky pushed a commit that referenced this pull request Nov 7, 2023
@AnthonyMikh @davidbarsky
appender: remove Sync bound from writer for NonBlocking (#2607)
19ab4fd 
## Motivation

`NonBlocking` from `tracing-appender` wraps a writer and requires that
writer to implement `Sync` (among other bounds). However it is not clear
why this bound is necessary in first place: this writer is sent to a
dedicated thread and never used directly concurrently.

#1934 demonstrates that it is a real problem for some people. Also at my
current work we hit this issue as well when a third-party writer did not
implement `Sync`. Our workaround was to wrap that writer in our own type
and manually implement `Send` and `Sync` for it. Needless to say that it
is more a hack than a proper solution.

## Solution

Remove `Sync` bound in relevant places. Yep, that simple. Probably
having it in first place was just an oversight.

Closes #1934
hawkw pushed a commit that referenced this pull request Nov 7, 2023
@AnthonyMikh @hawkw
appender: remove Sync bound from writer for NonBlocking (#2607)
8cdc9da 
## Motivation

`NonBlocking` from `tracing-appender` wraps a writer and requires that
writer to implement `Sync` (among other bounds). However it is not clear
why this bound is necessary in first place: this writer is sent to a
dedicated thread and never used directly concurrently.

#1934 demonstrates that it is a real problem for some people. Also at my
current work we hit this issue as well when a third-party writer did not
implement `Sync`. Our workaround was to wrap that writer in our own type
and manually implement `Send` and `Sync` for it. Needless to say that it
is more a hack than a proper solution.

## Solution

Remove `Sync` bound in relevant places. Yep, that simple. Probably
having it in first place was just an oversight.

Closes #1934
hawkw pushed a commit that referenced this pull request Nov 13, 2023
@davidbarsky
chore: prepare tracing-appender 0.2.3 release (#2790)
c6bedbe 
# 0.2.3 (November 13, 2023)

This release contains several new features. It also increases the
minimum supported Rust version (MSRV) to Rust 1.63.0.

## Added

- **rolling**: add option to automatically delete old log files (#2323)
- **non_blocking**: allow worker thread name to be configured (#2365)
- **rolling**: add a builder for constructing `RollingFileAppender`s
  (#2227)
- **rolling**: add `Builder::filename_suffix` parameter (#2225)
- **non_blocking**: remove `Sync` bound from writer for `NonBlocking`
  (#2607)
- **non_blocking**: name spawned threads (#2219)

## Fixed

- Fixed several documentation typos and issues (#2689, #2375)

## Changed

- Increased minimum supported Rust version (MSRV) to 1.63.0+ (#2793)
- Updated minimum `tracing-subscriber` version to
  [0.3.18][subscriber-v0.3.18] (#2790)

[subscriber-v0.3.18]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request Nov 21, 2023
@AnthonyMikh @kaffarell
appender: remove Sync bound from writer for NonBlocking (tokio-rs…
025cf19 
…#2607)

## Motivation

`NonBlocking` from `tracing-appender` wraps a writer and requires that
writer to implement `Sync` (among other bounds). However it is not clear
why this bound is necessary in first place: this writer is sent to a
dedicated thread and never used directly concurrently.

tokio-rs#1934 demonstrates that it is a real problem for some people. Also at my
current work we hit this issue as well when a third-party writer did not
implement `Sync`. Our workaround was to wrap that writer in our own type
and manually implement `Send` and `Sync` for it. Needless to say that it
is more a hack than a proper solution.

## Solution

Remove `Sync` bound in relevant places. Yep, that simple. Probably
having it in first place was just an oversight.

Closes tokio-rs#1934
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request Feb 14, 2024
@AnthonyMikh @kaffarell
appender: remove Sync bound from writer for NonBlocking (tokio-rs…
d17f3e9 
…#2607)

## Motivation

`NonBlocking` from `tracing-appender` wraps a writer and requires that
writer to implement `Sync` (among other bounds). However it is not clear
why this bound is necessary in first place: this writer is sent to a
dedicated thread and never used directly concurrently.

tokio-rs#1934 demonstrates that it is a real problem for some people. Also at my
current work we hit this issue as well when a third-party writer did not
implement `Sync`. Our workaround was to wrap that writer in our own type
and manually implement `Send` and `Sync` for it. Needless to say that it
is more a hack than a proper solution.

## Solution

Remove `Sync` bound in relevant places. Yep, that simple. Probably
having it in first place was just an oversight.

Closes tokio-rs#1934
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request Feb 14, 2024
@davidbarsky @kaffarell
chore: prepare tracing-appender 0.2.3 release (tokio-rs#2790)
80aa97c 
# 0.2.3 (November 13, 2023)

This release contains several new features. It also increases the
minimum supported Rust version (MSRV) to Rust 1.63.0.

## Added

- **rolling**: add option to automatically delete old log files (tokio-rs#2323)
- **non_blocking**: allow worker thread name to be configured (tokio-rs#2365)
- **rolling**: add a builder for constructing `RollingFileAppender`s
  (tokio-rs#2227)
- **rolling**: add `Builder::filename_suffix` parameter (tokio-rs#2225)
- **non_blocking**: remove `Sync` bound from writer for `NonBlocking`
  (tokio-rs#2607)
- **non_blocking**: name spawned threads (tokio-rs#2219)

## Fixed

- Fixed several documentation typos and issues (tokio-rs#2689, tokio-rs#2375)

## Changed

- Increased minimum supported Rust version (MSRV) to 1.63.0+ (tokio-rs#2793)
- Updated minimum `tracing-subscriber` version to
  [0.3.18][subscriber-v0.3.18] (tokio-rs#2790)

[subscriber-v0.3.18]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
@davidbarsky @kaffarell
chore: prepare tracing-appender 0.2.3 release (tokio-rs#2790)
a7717e9 
# 0.2.3 (November 13, 2023)

This release contains several new features. It also increases the
minimum supported Rust version (MSRV) to Rust 1.63.0.

## Added

- **rolling**: add option to automatically delete old log files (tokio-rs#2323)
- **non_blocking**: allow worker thread name to be configured (tokio-rs#2365)
- **rolling**: add a builder for constructing `RollingFileAppender`s
  (tokio-rs#2227)
- **rolling**: add `Builder::filename_suffix` parameter (tokio-rs#2225)
- **non_blocking**: remove `Sync` bound from writer for `NonBlocking`
  (tokio-rs#2607)
- **non_blocking**: name spawned threads (tokio-rs#2219)

## Fixed

- Fixed several documentation typos and issues (tokio-rs#2689, tokio-rs#2375)

## Changed

- Increased minimum supported Rust version (MSRV) to 1.63.0+ (tokio-rs#2793)
- Updated minimum `tracing-subscriber` version to
  [0.3.18][subscriber-v0.3.18] (tokio-rs#2790)

[subscriber-v0.3.18]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NonBlocking requires its inner writer to be Sync and I'm not sure why
2 participants
@AnthonyMikh @hawkw