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

Fix rotation of log files. #84

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mortenboye
Copy link

Using AdvancedFileOutput on Linux (Debian) I noticed that the rotation of log files did not work as expected.
latest.log did get renamed to whatever the configuration defined, but the logger would keep writing to this file.
This would lead to log files far exceeding the size defined with maxFileSizeKB.

This PR fixes this.

@Bungeefan
Copy link
Member

Hi, are you, by any chance, using different file systems for the latest.log and the archived/rotated logs?

https://api.flutter.dev/flutter/dart-io/File/rename.html

On some platforms, a rename operation cannot move a file between different file systems.

@mortenboye
Copy link
Author

Hi, are you, by any chance, using different file systems for the latest.log and the archived/rotated logs?

https://api.flutter.dev/flutter/dart-io/File/rename.html

On some platforms, a rename operation cannot move a file between different file systems.

Good point.
I am not. The files reside in the same folder on the same drive.

@Bungeefan
Copy link
Member

Then, how did you find your fix, just trial-and-error?

@mortenboye
Copy link
Author

Then, how did you find your fix, just trial-and-error?

Kindof.
I could see the archived file kept increasing in size, and looking at the code it made sense to me that renaming the _file reference would rename the file on disk, but keep pointing to the same file from code.
So copying the contents of latest and then clearing it before keeping on writing to it, seemed like an easy, yet naive solution.

I'm sure there are better ways of doing it :-)

@Bungeefan
Copy link
Member

Ah, ok. Did you test your fix, does it reliably fix your problem on Debian?

Yeah, I get why somebody would think that, even I had to check again:
In the default dart:io File implementation, the _path field is final and the rename method just returns a new File instance with the new path.
_path Field: https://github.com/dart-lang/sdk/blob/34fa3c1fb5da0751755061f35eebfd9c89dce09e/sdk/lib/io/file_impl.dart#L235
rename Method: https://github.com/dart-lang/sdk/blob/34fa3c1fb5da0751755061f35eebfd9c89dce09e/sdk/lib/io/file_impl.dart#L337

It is weird as it seems to be working just fine on Windows and Ubuntu (GitHub Actions).
So before accepting your fix, I would like to find out why it doesn't work for you and if more people are experiencing a similar behavior.

@mortenboye
Copy link
Author

I did not look deeper into the actual implementation details. But the proposed "fix" does solve the issue on my own system.

Please do the proper due diligence before considering this.

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