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

Update ANCM file monitoring to hopefully avoid unneeded shutdowns #57735

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Sep 6, 2024

Hoping it fixes issues like #55939 where it looks like a phantom app_offline.htm file is seen by ANCM which causes the app to shutdown.

There were three specific issues found in the code:

  1. m_detectedAppOffline wasn't initialized, so it could have any value, if it was true and there was a file notification then we'd trigger an app shutdown due to "detecting" app_offline
  2. We just assumed if cbCompletion (now bytesTransferred) was 0 that app_offline was added to the directory. That's not really a good assumption, so changed it to manually check for app_offline
  3. Any file or directory name that is a subset of app_offline.htm (e.g. app_o) would be detected as app_offline.htm

Also updated what file changes we look for to not include FILE_NOTIFY_CHANGE_SECURITY and FILE_NOTIFY_CHANGE_ATTRIBUTES (ref) to hopefully minimize the likelihood of getting bytesTransferred == 0 and it just seems like extra work when we aren't interested in those events.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Sep 6, 2024
Copy link
Member

@mgravell mgravell left a comment

Choose a reason for hiding this comment

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

looks reasonable to me

@amcasey
Copy link
Member

amcasey commented Sep 10, 2024

m_detectedAppOffline wasn't initialized

Are we not running a static analyzer to catch stuff like this?

@BrennanConroy
Copy link
Member Author

m_detectedAppOffline wasn't initialized

Are we not running a static analyzer to catch stuff like this?

That's going to be another PR, because we definitely should be and I was working on adding it, but it added way too many warnings for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants