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

logging: file: disable buffering on the output log files #9

Merged
merged 2 commits into from Mar 21, 2021

Conversation

ynezz
Copy link
Contributor

@ynezz ynezz commented Jan 7, 2021

I've observed following on Windows Server 2012 R2 running under VMWare
where the disk I/O is handled by VMWare Virtual disk SCSI Disk Device
storage.

Current line buffering makes live observing of log files with tail -f
quite unusable as the current line is always missing from the log file
until there is next line logged.

Sometimes log content is missing completly, usually when the running
application is terminated and the buffered log content is not flushed
properly to the disk.

Disabling buffering fixes the issues.

Signed-off-by: Petr Štetiar ynezz@true.cz

@ynezz
Copy link
Contributor Author

ynezz commented Jan 7, 2021

While at it, I've also added commit which fixes the CI. Quite strange, that I can't see the CI running on this PR, but it runs just fine in my fork.

@Tieske
Copy link
Member

Tieske commented Jan 7, 2021

Current line buffering makes live observing of log files with tail -f
quite unusable as the current line is always missing from the log file
until there is next line logged.

Is this only when using VMware?

Also what is the effect on performance when disabling buffering?

@ynezz
Copy link
Contributor Author

ynezz commented Jan 21, 2021

Is this only when using VMware?

I don't use Lua much on Windows, so I would need to test it.

Also what is the effect on performance when disabling buffering?

Do you've any specific benchmark in the mind I could run?

@Tieske
Copy link
Member

Tieske commented Mar 19, 2021

Had a look at this again, it seems to be a Windows specific problem.

If you look at the Lua implementation, it uses the standard C constants (_IONBF, _IOFBF, _IOLBF) to specify the buffer mode.

Now if you look that up in the Windows docs, you'll see this comment:

_IOLBF | For some systems, this provides line buffering. However, for Win32, the behavior is the same as _IOFBF - Full Buffering.

So It's just WIndows not supporting line-buffered mode.

As for this PR, how about at the top of the module testing for Windows, and set a buffering constant to either "line" (non-Windows), or "no" (Windows). And then use that constant value. Probably add some comments to explain the difference between the platforms.

@Tieske Tieske force-pushed the ynezz/win-fix-file-buffering branch from 3424974 to 4651862 Compare March 19, 2021 09:59
@Tieske
Copy link
Member

Tieske commented Mar 19, 2021

@ynezz I pushed a commit on your branch, implementing what I wrote above. Please have a look.

Any ideas as to why CI doesn't seem to run?

Use version 8.0.0 or greater as GitHub has deprecated older versions of
the actions core libraries.

References: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/
Signed-off-by: Petr Štetiar <ynezz@true.cz>
This causes file-log to not show the latest lines when tailing
log files on Windows. Only when the next line comes in the
previous one is actually written.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
@Tieske Tieske force-pushed the ynezz/win-fix-file-buffering branch from 4651862 to ebc335b Compare March 21, 2021 08:59
@Tieske
Copy link
Member

Tieske commented Mar 21, 2021

k, fixed the CI, the trigger for pull-requests wasn't in the flow.

@Tieske Tieske merged commit ad17dab into lunarmodules:master Mar 21, 2021
4 checks passed
@Tieske
Copy link
Member

Tieske commented Mar 21, 2021

thx @ynezz !!

@ynezz
Copy link
Contributor Author

ynezz commented Apr 6, 2021

@Tieske just found time to test it on the same VM, seems to be fine, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants