Relax restrictions on writer for tracing_appender::non_blocking::NonBlocking#2607
Merged
hawkw merged 2 commits intotokio-rs:masterfrom Oct 15, 2023
Merged
Conversation
hawkw
approved these changes
Oct 15, 2023
Member
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
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!
davidbarsky
pushed a commit
that referenced
this pull request
Nov 7, 2023
## 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
## 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
# 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
…#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
…#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
# 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
# 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
NonBlockingfromtracing-appenderwraps a writer and requires that writer to implementSync(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 implementSendandSyncfor it. Needless to say that it is more a hack than a proper solution.Solution
Remove
Syncbound in relevant places. Yep, that simple. Probably having it in first place was just an oversight.Closes #1934