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] Log stream delay when dir does not exist #174

Merged
merged 1 commit into from
May 23, 2024

Conversation

TinaMor
Copy link
Contributor

@TinaMor TinaMor commented May 21, 2024

What does this PR do?

This PR fixes LogMonitor tool missing the first few log lines when the log file and directory are created after the LogMonitor tool has started. Issue #170

This regression is caused by the PR #149. The WaitForMultipleObjects() function waits until the timer expires even though the object is already created during the wait interval.

Summary of Fix

The fix adds a waitable object on the directory that the LogMonitor tool is observing. The WaitForMultipleObjects() function waits until either the timer expires or the specified file is created. By including the handle of the waitable object in the array, the function can return as soon as the file is created.

This way, the system does not have to wait for the entire duration specified by the timer if the file is created during the wait period. This makes the file monitoring more responsive and efficient.

@TinaMor TinaMor marked this pull request as draft May 21, 2024 15:04
@TinaMor TinaMor marked this pull request as ready for review May 21, 2024 19:13
@TinaMor TinaMor force-pushed the fix-file-monitor-log-stream branch from ed1ea40 to aaf6953 Compare May 22, 2024 08:29
@TinaMor TinaMor force-pushed the fix-file-monitor-log-stream branch 3 times, most recently from 1acced4 to fc52e3d Compare May 22, 2024 09:37
@TinaMor TinaMor force-pushed the fix-file-monitor-log-stream branch from fc52e3d to 3fb1ee3 Compare May 22, 2024 09:39
@bobsira
Copy link
Contributor

bobsira commented May 23, 2024

File Test
I've just tested this change in a container and it appears everything runs fine! I'll do a locally run to see if there is any issue!

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Other than the minor convention nitpick, LGTM.

Just for the records purposes, could you give more description on the PR / commit, explaining a little about what was causing this delay and the fix?

class LogFileMonitor final
{
public:
LogFileMonitor() = delete;

LogFileMonitor(
_In_ const std::wstring& LogDirectory,
_In_ const std::wstring& Filter,
_In_ const std::wstring &LogDirectory,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I think the & should right after the type as per common convention; T&?

Copy link
Contributor

@bobsira bobsira left a comment

Choose a reason for hiding this comment

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

This change looks good to me. I have consistent behavior when I tested it on my host and a container!

@TinaMor TinaMor merged commit c524905 into microsoft:main May 23, 2024
6 checks passed
@TinaMor TinaMor deleted the fix-file-monitor-log-stream branch May 23, 2024 17:12
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.

5 participants