Skip to content

#69: Fix inotify watch problem + update to .NET 7 - #70

Merged
hishamco merged 2 commits into
hishamco:devfrom
nbokmans:dev
Jan 31, 2023
Merged

hishamco merged 2 commits into
hishamco:devfrom
nbokmans:dev

Conversation

@nbokmans

@nbokmans nbokmans commented Jan 31, 2023 •

Copy link
Copy Markdown
Contributor

This PR attempts to solve #69

@nbokmans nbokmans changed the title Fix inotify watch problem + update to .NET 7 #69: Fix inotify watch problem + update to .NET 7 Jan 31, 2023
{
var builder = new ConfigurationBuilder()
.AddJsonFile(filePath, optional: true, reloadOnChange: true);
.AddJsonFile(filePath, optional: true, reloadOnChange: false);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This change is dangerous, so whever your change the JSON file it will not reload the changes

@nbokmans nbokmans Jan 31, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is precisely what I am trying to fix. The reloadOnChange: true flag seems to be bugged in ASP.NET on Linux, in which it does not release the inotify instances for the file system watcher. On production webservers where a limit is configured (very common), this will eventually make it so your app stops responding. Changing it to reloadOnChange: false makes it so no inotify instances are created for the JSON resource file you are trying to read. Also I figured that generally speaking, in staging/production environments, the json resource files will not change during runtime. I've linked some related reading material in the issue description here: #69 and dotnet/runtime#62869 and https://www.travisgosselin.com/configured-user-limit-inotify-instances/

@hishamco hishamco closed this Jan 31, 2023
@hishamco hishamco reopened this Jan 31, 2023
@hishamco

Copy link
Copy Markdown
Owner

Just close and reopen to trigger the build again

@hishamco
hishamco merged commit ad73872 into hishamco:dev Jan 31, 2023
@hishamco

hishamco commented Jan 31, 2023 •

Copy link
Copy Markdown
Owner

Thanks for your contribution, we might need to release 3.0.1 ASAP

@hishamco

Copy link
Copy Markdown
Owner

FYI https://www.nuget.org/packages/My.Extensions.Localization.Json/3.0.1

@suchoss

suchoss commented Feb 19, 2023

Copy link
Copy Markdown

But thus us not a fix - it is just a workaround... What if we need to have reloadOnChange = true?

This solves nothing

@hishamco

Copy link
Copy Markdown
Owner

Waiting for a proper fix

@nbokmans

Copy link
Copy Markdown
Contributor Author

@suchoss feel free to come up with a better solution! I think I've argued clearly why reloadOnChange should not be necessary in production environments. Besides that, I think the core problem lies in how ASP.NET Core is handling file watches, not this library specifically. So I think that the proper fix can only be done by aspnetcore team.

You can always revert back to older version v2 where reloadOnChange behavior hasn't changed.

@suchoss

suchoss commented Feb 19, 2023

Copy link
Copy Markdown

Fair enough, I understand that we need ASP.NET core team fixing this issue.

@hishamco

Copy link
Copy Markdown
Owner

Exactly

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.

3 participants