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

appender: impl MakeWriter for RollingFileAppender #1760

Merged
merged 5 commits into from
Dec 17, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Nov 30, 2021

Motivation

Currently, tracing-appender's RollingFileAppender does not implement
the MakeWriter trait. This means it can only be used by either
wrapping it in NonBlocking, or by wrapping it in a Mutex. However,
this shouldn't be strictly necessary, as &File implements io::Write.
It should thus only be necessary to introduce locking when we are in the
process of rotating the log file.

Solution

This branch adds a MakeWriter implementation for
RollingFileAppender. This is done by moving the file itself inside of
an RwLock, so that a read lock is acquired to write to the file. This
allows multiple threads to write to the file without contention. When
the file needs to be rolled, the rolling thread acquires the write lock
to replace the file. Acquiring the write lock is guarded by an atomic
CAS on the timestamp, so that only a single thread will try to roll the
file. This prevents other threads from immediately rolling the file
again when the write lock is released.

I...should probably write tests for that, though.

Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw requested a review from davidbarsky November 30, 2021 22:49
@hawkw hawkw requested a review from a team as a code owner November 30, 2021 22:49
Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

LGTM

@hawkw hawkw merged commit 6dd9d23 into master Dec 17, 2021
@hawkw hawkw deleted the eliza/rolling-makewriter branch December 17, 2021 22:50
hawkw added a commit that referenced this pull request Dec 19, 2021
## Motivation

Currently, `tracing-appender`'s `RollingFileAppender` does not implement
the `MakeWriter` trait. This means it can only be used by either
wrapping it in `NonBlocking`, or by wrapping it in a `Mutex`. However,
this shouldn't be strictly necessary, as `&File` implements `io::Write`.
It should thus only be necessary to introduce locking when we are in the
process of _rotating_ the log file.

## Solution

This branch adds a `MakeWriter` implementation for
`RollingFileAppender`. This is done by moving the file itself inside of
an `RwLock`, so that a read lock is acquired to write to the file. This
allows multiple threads to write to the file without contention. When
the file needs to be rolled, the rolling thread acquires the write lock
to replace the file. Acquiring the write lock is guarded by an atomic
CAS on the timestamp, so that only a single thread will try to roll the
file. This prevents other threads from immediately rolling the file
_again_ when the write lock is released.

I...should probably write tests for that, though.

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this pull request Dec 19, 2021
## Motivation

Currently, `tracing-appender`'s `RollingFileAppender` does not implement
the `MakeWriter` trait. This means it can only be used by either
wrapping it in `NonBlocking`, or by wrapping it in a `Mutex`. However,
this shouldn't be strictly necessary, as `&File` implements `io::Write`.
It should thus only be necessary to introduce locking when we are in the
process of _rotating_ the log file.

## Solution

This branch adds a `MakeWriter` implementation for
`RollingFileAppender`. This is done by moving the file itself inside of
an `RwLock`, so that a read lock is acquired to write to the file. This
allows multiple threads to write to the file without contention. When
the file needs to be rolled, the rolling thread acquires the write lock
to replace the file. Acquiring the write lock is guarded by an atomic
CAS on the timestamp, so that only a single thread will try to roll the
file. This prevents other threads from immediately rolling the file
_again_ when the write lock is released.

I...should probably write tests for that, though.

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this pull request Dec 20, 2021
## Motivation

Currently, `tracing-appender`'s `RollingFileAppender` does not implement
the `MakeWriter` trait. This means it can only be used by either
wrapping it in `NonBlocking`, or by wrapping it in a `Mutex`. However,
this shouldn't be strictly necessary, as `&File` implements `io::Write`.
It should thus only be necessary to introduce locking when we are in the
process of _rotating_ the log file.

## Solution

This branch adds a `MakeWriter` implementation for
`RollingFileAppender`. This is done by moving the file itself inside of
an `RwLock`, so that a read lock is acquired to write to the file. This
allows multiple threads to write to the file without contention. When
the file needs to be rolled, the rolling thread acquires the write lock
to replace the file. Acquiring the write lock is guarded by an atomic
CAS on the timestamp, so that only a single thread will try to roll the
file. This prevents other threads from immediately rolling the file
_again_ when the write lock is released.

I...should probably write tests for that, though.

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this pull request Feb 28, 2022
# 0.2.1 (February 28, 2022)

This release adds an implementation of the `MakeWriter` trait for
`RollingFileAppender`, allowing it to be used without wrapping in a
`NonBlocking` writer.

This release increases the minimum supported Rust version to 1.53.0.

### Added

- **rolling**: Added `MakeWriter` implementation for
  `RollingFileAppender` ([#1760])

### Changed

- Updated minimum supported Rust version (MSRV) to 1.53.0 ([#1851])
- `parking_lot`: updated to v0.12 ([#1878])

### Fixed

- Fixed several documentation typos and issues ([#1780], [d868054],
  [#1943])

[#1760]: #1760
[#1851]: #1851
[#1878]: #1878
[#1943]: #1943
[d868054]: d868054
hawkw added a commit that referenced this pull request Feb 28, 2022
# 0.2.1 (February 28, 2022)

This release adds an implementation of the `MakeWriter` trait for
`RollingFileAppender`, allowing it to be used without wrapping in a
`NonBlocking` writer.

This release increases the minimum supported Rust version to 1.53.0.

### Added

- **rolling**: Added `MakeWriter` implementation for
  `RollingFileAppender` ([#1760])

### Changed

- Updated minimum supported Rust version (MSRV) to 1.53.0 ([#1851])
- `parking_lot`: updated to v0.12 ([#1878])

### Fixed

- Fixed several documentation typos and issues ([#1780], [d868054],
  [#1943])

[#1760]: #1760
[#1851]: #1851
[#1878]: #1878
[#1943]: #1943
[d868054]: d868054
hawkw added a commit that referenced this pull request Feb 28, 2022
# 0.2.1 (February 28, 2022)

This release adds an implementation of the `MakeWriter` trait for
`RollingFileAppender`, allowing it to be used without wrapping in a
`NonBlocking` writer.

This release increases the minimum supported Rust version to 1.53.0.

### Added

- **rolling**: Added `MakeWriter` implementation for
  `RollingFileAppender` ([#1760])

### Changed

- Updated minimum supported Rust version (MSRV) to 1.53.0 ([#1851])
- `parking_lot`: updated to v0.12 ([#1878])

### Fixed

- Fixed several documentation typos and issues ([#1780], [d868054],
  [#1943])

[#1760]: #1760
[#1851]: #1851
[#1878]: #1878
[#1943]: #1943
[d868054]: d868054
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
## Motivation

Currently, `tracing-appender`'s `RollingFileAppender` does not implement
the `MakeWriter` trait. This means it can only be used by either
wrapping it in `NonBlocking`, or by wrapping it in a `Mutex`. However,
this shouldn't be strictly necessary, as `&File` implements `io::Write`.
It should thus only be necessary to introduce locking when we are in the
process of _rotating_ the log file.

## Solution

This branch adds a `MakeWriter` implementation for
`RollingFileAppender`. This is done by moving the file itself inside of
an `RwLock`, so that a read lock is acquired to write to the file. This
allows multiple threads to write to the file without contention. When
the file needs to be rolled, the rolling thread acquires the write lock
to replace the file. Acquiring the write lock is guarded by an atomic
CAS on the timestamp, so that only a single thread will try to roll the
file. This prevents other threads from immediately rolling the file
_again_ when the write lock is released.

I...should probably write tests for that, though.

Signed-off-by: Eliza Weisman <[email protected]>
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.2.1 (February 28, 2022)

This release adds an implementation of the `MakeWriter` trait for
`RollingFileAppender`, allowing it to be used without wrapping in a
`NonBlocking` writer.

This release increases the minimum supported Rust version to 1.53.0.

### Added

- **rolling**: Added `MakeWriter` implementation for
  `RollingFileAppender` ([tokio-rs#1760])

### Changed

- Updated minimum supported Rust version (MSRV) to 1.53.0 ([tokio-rs#1851])
- `parking_lot`: updated to v0.12 ([tokio-rs#1878])

### Fixed

- Fixed several documentation typos and issues ([tokio-rs#1780], [d868054],
  [tokio-rs#1943])

[tokio-rs#1760]: tokio-rs#1760
[tokio-rs#1851]: tokio-rs#1851
[tokio-rs#1878]: tokio-rs#1878
[tokio-rs#1943]: tokio-rs#1943
[d868054]: tokio-rs@d868054
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.

2 participants