Skip to content

LogFactory - KeepVariablesOnReload = true by default for config variables#4240

Merged
snakefoot merged 3 commits intoNLog:devfrom
snakefoot:KeepVariablesOnReloadTrue
Jan 17, 2021
Merged

LogFactory - KeepVariablesOnReload = true by default for config variables#4240
snakefoot merged 3 commits intoNLog:devfrom
snakefoot:KeepVariablesOnReloadTrue

Conversation

@snakefoot
Copy link
Copy Markdown
Contributor

@snakefoot snakefoot commented Jan 14, 2021

Resolves #2509 and resolves #2960

The old way triggered variable-restore AFTER having loaded the new config from file. It would restore ALL values from the old config (Thus discarding any updates made in the new config files). And at the same time prevents static-mode variables from working with config-variables assigned from API.

The new way triggers variable-restore BEFORE starting to load the new file. It will ONLY restore config-variables that has been assigned from the API by using LoggingConfiguration.Variables-property.

  • This means changes to config-variables in the new config file, will no longer be discarded (unless the config-variable has been assigned from the API). Less surprises when having KeepVariablesOnReload=true by default.
  • This means use of config-variables in static-mode (without NLog Layout) will now apply values assigned from the API.

The new way will work automatically when manually calling XmlLoggingConfiguration.Reload(), instead of only when file-watcher detects file-changes and triggers special reload.

The new way will also automatically add extra protection for NLog-Config-Variable-Layout that is not ThreadSafe by default (Rare case). Thus removing the possible issue with thread-safety and NLog-Config-Variables.

  • The ThreadSafe-wrapper is hidden from the user, and is only used by ${var} or when assigned as const-Layout.

@snakefoot snakefoot added this to the 5.0 (new) milestone Jan 14, 2021
@snakefoot snakefoot force-pushed the KeepVariablesOnReloadTrue branch 4 times, most recently from 9f48797 to cd6c628 Compare January 14, 2021 22:07
@snakefoot
Copy link
Copy Markdown
Contributor Author

snakefoot commented Jan 14, 2021

@304NotModified Not the smallest PullRequest for changing a default-value. But I find it important that NLog Config Variables behaves in a predictable manner, where only NLog Config Variables that have been modified by API are kept on reload.

@304NotModified
Copy link
Copy Markdown
Member

The description sounds good!

@304NotModified
Copy link
Copy Markdown
Member

I'm fine with this enabled by default. I haven't checked the code yet.

@snakefoot snakefoot force-pushed the KeepVariablesOnReloadTrue branch 2 times, most recently from 3d24249 to 3632a2b Compare January 16, 2021 10:54
@snakefoot
Copy link
Copy Markdown
Contributor Author

Close and reopening to trigger rebuild

@snakefoot snakefoot closed this Jan 16, 2021
@snakefoot snakefoot reopened this Jan 16, 2021
@snakefoot snakefoot force-pushed the KeepVariablesOnReloadTrue branch from 3632a2b to 5fc6fa9 Compare January 16, 2021 13:51
@snakefoot snakefoot closed this Jan 16, 2021
@snakefoot snakefoot reopened this Jan 16, 2021
@304NotModified
Copy link
Copy Markdown
Member

Restarted azure pipeline

@304NotModified
Copy link
Copy Markdown
Member

Green!

@snakefoot
Copy link
Copy Markdown
Contributor Author

snakefoot commented Jan 16, 2021 via email

Comment thread src/NLog/Config/LoggingConfiguration.cs Outdated
Comment thread src/NLog/Config/LoggingConfigurationParser.cs Outdated
Comment thread src/NLog/Internal/Collections/ConfigVariablesDictionary.cs
private readonly ThreadSafeDictionary<string, Layout> _variables = new ThreadSafeDictionary<string, Layout>(StringComparer.OrdinalIgnoreCase);
private readonly LoggingConfiguration _configuration;
private ThreadSafeDictionary<string, Layout> _dynamicVariables;
private ThreadSafeDictionary<string, bool> _apiVariables;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Aren't those then the "const variables"?

Copy link
Copy Markdown
Contributor Author

@snakefoot snakefoot Jan 17, 2021

Choose a reason for hiding this comment

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

_dynamicVariables are those used by ${var} and const-variables. _apiVariables are those assigned using LoggingConfiguration.Variables-dictionary

Comment thread src/NLog/Internal/Collections/ConfigVariablesDictionary.cs
@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

84.8% 84.8% Coverage
0.0% 0.0% Duplication

@snakefoot snakefoot merged commit 617ac71 into NLog:dev Jan 17, 2021
@snakefoot
Copy link
Copy Markdown
Contributor Author

Updated wiki: https://github.com/NLog/NLog/wiki/Configuration-file with KeepVariablesOnReload=true by default

@snakefoot snakefoot added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Nov 27, 2021
@snakefoot snakefoot deleted the KeepVariablesOnReloadTrue branch July 30, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking behavior change Same API, different result documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) enhancement Improvement on existing feature needs documentation on wiki new default (breaking) Kind of Breaking behavior change nlog-configuration size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NLog variables are not threadsafe keepVariablesOnReload enabled by default

2 participants