Sherlock0203 commented on code in PR #3687:
URL: https://github.com/apache/brpc/pull/3687#discussion_r4511348799
##########
src/butil/logging.cc:
##########
@@ -425,6 +458,210 @@ void CloseLogFileUnlocked() {
CloseFile(log_file);
log_file = nullptr;
+ log_file_size = 0;
+}
+
+// ---------------------------- Log rotation ----------------------------
+// Rotation is size based and disabled by default. Once the current log file
+// is about to grow beyond --log_rotate_size_mb, it is renamed to
+// "<log_file_name>.1" and a fresh file is opened under the original name.
+// Existing backups are shifted down (".1" -> ".2" -> ...) beforehand and the
+// ones beyond --log_rotate_max_backups are removed, so that ".1" is always
+// the most recent backup. Every rotation also removes the backups left over
+// from a previously larger --log_rotate_max_backups, so that lowering the
+// flag at run time really lowers the number of files kept.
+//
+// A log file that disappeared from under the process is not a rotation
+// failure: the handle is closed so that the next write starts a fresh file,
+// and the backups are left untouched. The size at which it happened is
+// remembered all the same, so that a file which is still over the limit
+// once reopened (the ".\\debug.log" that Windows falls back to when the
+// requested name could not be opened at all) is not closed and reopened on
+// every single log.
+//
+// A rename that fails does not disable rotation for good, and a backup is
+// only ever replaced by a shift that succeeded: a backup that cannot be
+// moved out of the way aborts the rotation instead of being silently
+// overwritten with the current file. The size at which the rotation failed
+// is remembered and rotation is retried once the file has grown by another
+// --log_rotate_size_mb, which keeps a directory that cannot be written to
+// from closing, reopening and renaming the file for every single log. A
+// file that turns out to be smaller than it was at that point has been
+// replaced or truncated from the outside, and rotation resumes immediately.
+//
+// Sharing one log file between multiple processes is NOT supported together
+// with rotation: on POSIX LoggingLock only serializes the threads of a single
+// process, and after the rename the other processes would keep writing to the
+// renamed inode.
+
+// Return "<path>.<index>".
+PathString MakeBackupLogPath(const PathString& path, int index) {
+#if defined(OS_WIN)
+ wchar_t suffix[16];
+ swprintf(suffix, 16, L".%d", index);
+#else
+ char suffix[16];
+ snprintf(suffix, sizeof(suffix), ".%d", index);
+#endif
+ return path + suffix;
+}
+
+bool PathExists(const PathString& path) {
+#if defined(OS_WIN)
+ return GetFileAttributes(path.c_str()) != INVALID_FILE_ATTRIBUTES;
+#else
+ return ::access(path.c_str(), F_OK) == 0;
+#endif
+}
+
+bool RenameFilePath(const PathString& from, const PathString& to) {
+#if defined(OS_WIN)
+ return ::MoveFileEx(from.c_str(), to.c_str(),
+ MOVEFILE_REPLACE_EXISTING) != 0;
+#elif defined(OS_NACL)
+ // Do nothing; rename() isn't supported on NaCl.
+ return false;
+#else
+ return ::rename(from.c_str(), to.c_str()) == 0;
+#endif
+}
+
+// Narrow representation of a path, for printing. Paths are wide on Windows.
+std::string PathAsString(const PathString& path) {
+#if defined(OS_WIN)
+ return butil::WideToUTF8(path);
+#else
+ return path;
+#endif
+}
+
+// Back off after a rotation that could not rename `from' to `to': the size
+// that triggered it does not change by itself, so retrying on the next log
+// would close, reopen and rename the file for every single log. The failure
+// is printed once rather than per log. The caller must hold the logging
+// lock and `err' must have been captured before anything else could
+// overwrite it.
+void BackOffAfterFailedRotation(int64_t size_before_rotation,
+ const PathString& from,
+ const PathString& to,
+ SystemErrorCode err) {
+ failed_rotation_size = size_before_rotation;
+ if (rotation_failure_reported) {
+ return;
+ }
+ rotation_failure_reported = true;
+ fprintf(stderr, "Fail to rotate %s: renaming %s to %s failed: %s, "
+ "retrying after another --log_rotate_size_mb of logs\n",
+ PathAsString(*log_file_name).c_str(), PathAsString(from).c_str(),
+ PathAsString(to).c_str(),
+ SystemErrorCodeToString(err).c_str());
+}
+
+// Rename the current log file to "<log_file_name>.1" and shift the older
+// backups down by one. The caller must hold the logging lock.
+void RotateLogFileUnlocked() {
+ if (!log_file_name) {
+ return;
+ }
+ // Remember it before closing, CloseLogFileUnlocked() resets the size.
+ const int64_t size_before_rotation = log_file_size;
+ if (!PathExists(*log_file_name)) {
+ // The file was renamed or removed from the outside. There is nothing
+ // to rotate: closing the handle makes the next write start a fresh
+ // file under the original name, which is what a rotation would have
+ // done anyway. The backups are left alone, shifting them down would
+ // consume one generation for nothing. This is not an error, but the
+ // back off still applies: the file that the next write reopens may
+ // still be over the limit, which is the case of the Windows
+ // fallback to ".\\debug.log" when the requested name could not be
+ // opened at all. A fresh file is smaller than
+ // `size_before_rotation', which lets the next write retry at once.
+ CloseLogFileUnlocked();
+ failed_rotation_size = size_before_rotation;
+ return;
+ }
+ // Close before renaming: on Windows an opened file cannot be renamed, and
+ // on POSIX the handle would keep pointing at the renamed inode.
+ CloseLogFileUnlocked();
Review Comment:
Confirmed with your setup (file 0400, directory 0500): both the triggering
record and the next one were dropped. The early close was really only needed
on
Windows, where an opened file can't be renamed, and I had applied it on both
platforms. On POSIX the rotation now renames the file while the handle is
still
open, and the old handle is closed only after the fresh one is in place; if
the
path can't be reopened, the backup is moved back under its original name and
the
old handle keeps appending, so nothing is lost either way. Windows still
closes
before the rename, but a failed rename reattaches the handle to the file
that's
still sitting there, and a successful one reopens immediately — the remaining
window is the single log that triggered the rotation. keeps_writing_when_the_
file_can_not_be_reopened builds your scenario (obstacle on the shift plus
chmod
0400 on the current file) and checks that the file keeps growing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
