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

Improving windows support #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomekkolo
Copy link

  • added removal of also \r when read line ends with \r\n
  • changed the way files are open on windows. By default, golang on windows opens a file without FILE_SHARE_DELETE flag, what prevents
    deleting a file when go-tail is reading it. Used elastic beats ReadOpen implementation of opening files.

* added removal of also \r when read line ends with \r\n
* changed the way files are open on windows. By default, golang on windows opens a file without FILE_SHARE_DELETE flag, what prevents
deleting a file when go-tail is reading it. Used elastic beats ReadOpen implementation of opening files.
Copy link
Contributor

@markdascher markdascher left a comment

Choose a reason for hiding this comment

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

This looks solid overall. My only hesitation is with pulling in libbeat for this tiny feature.

Unfortunately, the only alternative is to copy Go's entire implementation of os.Open, just replacing the sharing mode bits. So pulling in libbeat is probably the way to go.

Could you describe a real-world situation where having the FILE_SHARE_DELETE sharing mode would've made a difference? I just want to make sure we fully understand the problem.

@tomekkolo
Copy link
Author

@markdascher the real use case, is that when you start tailing a file on windows, then this file is blocked for the removal/renaming by go tail because it was not open with FILE_SHARE_DELETE flag. So if any producer of the log file would like to rotate it (rename, delete) for example, it will not be possible on Windows.

@markdascher
Copy link
Contributor

It sounds like rotation would still be tricky, because of this issue. The producer of the log file would have to rename the log file prior to deleting it, or else Windows still won't let the same filename be reused for a new file.

...it won't work since FILE_SHARE_DELETE does not allow to create new file with original file name if handle of the file is not closed.

go-tail will eventually let go of the file handle, but not right away.

On the other hand, it seems Microsoft has promised to change that behavior, perhaps even doing so already for the latest Windows release. And there's some discussion in that same Go thread about making FILE_SHARE_DELETE the default for all Go programs.

Here's a test I whipped up in PowerShell:

New-Item nothing
$handle = [IO.File]::Open("nothing", [IO.FileMode]::Open, [IO.FileAccess]::Read, [IO.FileShare]::ReadWrite -bor [IO.FileShare]::Delete)
Remove-Item nothing
New-Item nothing
Remove-Item nothing
$handle.Close()

[System.Environment]::OSVersion.Version

(For the clearest results, paste by right-clicking, not CTRL+V.)

In the latest version of Windows, it was able to create a new file with the same name, even though I still had the old handle open:

Major  Minor  Build  Revision
-----  -----  -----  --------
10     0      18362  0

But that's the absolute cutting edge. I'd be curious if anyone's able to produce a different result on a different Windows release.

@tomekkolo
Copy link
Author

Seems that you are right, with

Major Minor Build Revision


10 0 17763 0

error is returned on attempt to create file when handle is still open. Removing is allowed. Also, with rename it works.

Nevertheless I wonder if this is really relevant for most apps. I.e. log4net seems to first close the file and rename before opening new one. https://github.com/apache/logging-log4net/blob/c04a774240fd4500ed3206124aba5b4bc8bc4933/src/Appender/RollingFileAppender.cs#L1210 . I think that logrotate does similar thing.

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