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

util: avoid panic in RotatingFileLogger #4492

Merged
merged 5 commits into from Apr 10, 2019

Conversation

Projects
None yet
6 participants
@kennytm
Copy link
Contributor

kennytm commented Apr 9, 2019

What have you changed? (mandatory)

Refactored RotatingFileLogger such that the file member can never be None, using the fact that it's fine to rename an open file on Unix. This removed all .unwrap()s and assert!s associated with file, which may be triggered if rotate() errored in the middle.

Additionally, rewritten rotation_file_path_with_timestamp() so it won't panic even if the log path contains non-UTF-8 bytes (won't happen in reality since the config is a String).

Now the only source of panic is when log_file is literally "/" or from the standard library.

On drop() we just flush the file and ignore any error, which fixes #4312.

What are the type of the changes? (mandatory)

  • Bug fix (change which fixes an issue)

How has this PR been tested? (mandatory)

Unit test.

Does this PR affect documentation (docs) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

util: prevents RotatingFileLogger from panicking at all
Signed-off-by: kennytm <kennytm@gmail.com>
Show resolved Hide resolved src/util/logger/file_log.rs Outdated
util: no need to create temporary file
Signed-off-by: kennytm <kennytm@gmail.com>

@kennytm kennytm force-pushed the kennytm:fix-4312 branch from 5035698 to 00b77ea Apr 9, 2019

@BusyJay

BusyJay approved these changes Apr 9, 2019

self.close()?;
self.flush()?;

// Note: renaming files while they're open only works on Linux and macOS.

This comment has been minimized.

Copy link
@Hoverbear

Hoverbear Apr 9, 2019

Member

Why not close it? Seems it's more compatible.

This comment has been minimized.

Copy link
@kennytm

kennytm Apr 9, 2019

Author Contributor

So that even if the new file failed to be opened for whatever reason we could still write logs to the old file. Plus File::close doesn't exist 🙃

Hoverbear and others added some commits Apr 9, 2019

@zhouqiang-cl

This comment has been minimized.

Copy link
Contributor

zhouqiang-cl commented Apr 10, 2019

/test

@kennytm kennytm added the S: LGT2 label Apr 10, 2019

@kennytm

This comment has been minimized.

Copy link
Contributor Author

kennytm commented Apr 10, 2019

/run-integration-tests

@kennytm

This comment has been minimized.

Copy link
Contributor Author

kennytm commented Apr 10, 2019

/run-integration-ddl-test

👀

dial tcp 127.0.0.1:5000: connect: connection refused ("dial tcp 127.0.0.1:5000: connect: connection refused")

@kennytm kennytm merged commit 4a06994 into tikv:master Apr 10, 2019

5 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@kennytm kennytm deleted the kennytm:fix-4312 branch Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.