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.

Sign up for GitHub

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

Jump to bottom

Expose logger properties on Logger #3424

Closed
wants to merge 3 commits into from
Closed

Expose logger properties on Logger #3424

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c73ee51
refactor
304NotModified May 24, 2019
554c142
expose
304NotModified May 24, 2019
a3110a6
to readonly
304NotModified May 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 26 additions & 21 deletions src/NLog/Logger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ protected internal Logger()
/// </summary>
public LogFactory Factory { get; private set; }

/// <summary>
/// Properties added with <see cref="WithProperty"/> or <see cref="SetProperty"/>
/// </summary>
public IDictionary<string, object> Properties => _contextProperties ?? new Dictionary<string, object>();
Copy link
Contributor

@snakefoot snakefoot May 24, 2019 •
edited

Choose a reason for hiding this comment

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

You are opening a door to race-condition-hell by returning an unprotected dictionary.

I recommend that you return IReadOnlyDictionary that only works on the platforms where it is known.

Copy link
Member Author

Choose a reason for hiding this comment

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

And that race condition isn't there with WithProperty/SetProperty

Copy link
Contributor

Choose a reason for hiding this comment

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

I know WithProperty/SetProperty are nice and sweet (because I made them so). But the evil user that starts calling the IDictionary-inteface to insert/remove elements will break everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't see what it should break. Could you please give an example ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Very confused that you don't know what happens if one enumerates a collection while another modifies the collection. Made some pseudo-code in new comment.


/// <summary>
/// Gets a value indicating whether logging is enabled for the specified level.
/// </summary>
Expand Down Expand Up @@ -110,7 +115,8 @@ public Logger WithProperty(string propertyKey, object propertyValue)

Logger newLogger = Factory.CreateNewLogger(GetType()) ?? new Logger();
newLogger.Initialize(Name, _configuration, Factory);
newLogger._contextProperties = CopyOnWrite(propertyKey, propertyValue);
newLogger._contextProperties = CreateContextPropertiesDictionary(_contextProperties);
newLogger._contextProperties[propertyKey] = propertyValue;
newLogger._contextLogger = _contextLogger; // Use the LoggerConfiguration of the parent Logger
return newLogger;
}
Expand All @@ -128,16 +134,15 @@ public void SetProperty(string propertyKey, object propertyValue)
if (string.IsNullOrEmpty(propertyKey))
throw new ArgumentException(nameof(propertyKey));

_contextProperties = CopyOnWrite(propertyKey, propertyValue);
_contextProperties = CreateContextPropertiesDictionary(_contextProperties);
_contextProperties[propertyKey] = propertyValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not good. As you are modifying the active dictionary.

Copy link
Member Author

Choose a reason for hiding this comment

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

not really, CreateContextPropertiesDictionary made a new reference. But anyway, I see this PR is already superseded 👼

}

private Dictionary<string, object> CopyOnWrite(string propertyKey, object propertyValue)
private static Dictionary<string, object> CreateContextPropertiesDictionary(Dictionary<string, object> contextProperties)
{
var contextProperties = _contextProperties;
contextProperties = contextProperties != null
? new Dictionary<string, object>(contextProperties)
: new Dictionary<string, object>();
contextProperties[propertyKey] = propertyValue;
return contextProperties;
}

Expand Down Expand Up @@ -246,10 +251,10 @@ public void LogException(LogLevel level, [Localizable(false)] string message, Ex
/// <param name="args">Arguments to format.</param>
[MessageTemplateFormatMethod("message")]
public void Log(LogLevel level, IFormatProvider formatProvider, [Localizable(false)] string message, params object[] args)
{
{
if (IsEnabled(level))
{
WriteToTargets(level, formatProvider, message, args);
WriteToTargets(level, formatProvider, message, args);
}
}

Expand All @@ -258,8 +263,8 @@ public void Log(LogLevel level, IFormatProvider formatProvider, [Localizable(fal
/// </summary>
/// <param name="level">The log level.</param>
/// <param name="message">Log message.</param>
public void Log(LogLevel level, [Localizable(false)] string message)
{
public void Log(LogLevel level, [Localizable(false)] string message)
{
if (IsEnabled(level))
{
WriteToTargets(level, null, message);
Expand All @@ -273,8 +278,8 @@ public void Log(LogLevel level, [Localizable(false)] string message)
/// <param name="message">A <see langword="string" /> containing format items.</param>
/// <param name="args">Arguments to format.</param>
[MessageTemplateFormatMethod("message")]
public void Log(LogLevel level, [Localizable(false)] string message, params object[] args)
{
public void Log(LogLevel level, [Localizable(false)] string message, params object[] args)
{
if (IsEnabled(level))
{
WriteToTargets(level, message, args);
Expand Down Expand Up @@ -340,10 +345,10 @@ public void Log(LogLevel level, Exception exception, IFormatProvider formatProvi
/// <param name="argument">The argument to format.</param>
[MessageTemplateFormatMethod("message")]
public void Log<TArgument>(LogLevel level, IFormatProvider formatProvider, [Localizable(false)] string message, TArgument argument)
{
{
if (IsEnabled(level))
{
WriteToTargets(level, formatProvider, message, new object[] { argument });
WriteToTargets(level, formatProvider, message, new object[] { argument });
}
}

Expand Down Expand Up @@ -374,11 +379,11 @@ public void Log<TArgument>(LogLevel level, [Localizable(false)] string message,
/// <param name="argument1">The first argument to format.</param>
/// <param name="argument2">The second argument to format.</param>
[MessageTemplateFormatMethod("message")]
public void Log<TArgument1, TArgument2>(LogLevel level, IFormatProvider formatProvider, [Localizable(false)] string message, TArgument1 argument1, TArgument2 argument2)
{
public void Log<TArgument1, TArgument2>(LogLevel level, IFormatProvider formatProvider, [Localizable(false)] string message, TArgument1 argument1, TArgument2 argument2)
{
if (IsEnabled(level))
{
WriteToTargets(level, formatProvider, message, new object[] { argument1, argument2 });
WriteToTargets(level, formatProvider, message, new object[] { argument1, argument2 });
}
}

Expand All @@ -393,7 +398,7 @@ public void Log<TArgument>(LogLevel level, [Localizable(false)] string message,
/// <param name="argument2">The second argument to format.</param>
[MessageTemplateFormatMethod("message")]
public void Log<TArgument1, TArgument2>(LogLevel level, [Localizable(false)] string message, TArgument1 argument1, TArgument2 argument2)
{
{
if (IsEnabled(level))
{
WriteToTargets(level, message, new object[] { argument1, argument2 });
Expand All @@ -413,11 +418,11 @@ public void Log<TArgument>(LogLevel level, [Localizable(false)] string message,
/// <param name="argument2">The second argument to format.</param>
/// <param name="argument3">The third argument to format.</param>
[MessageTemplateFormatMethod("message")]
public void Log<TArgument1, TArgument2, TArgument3>(LogLevel level, IFormatProvider formatProvider, [Localizable(false)] string message, TArgument1 argument1, TArgument2 argument2, TArgument3 argument3)
{
public void Log<TArgument1, TArgument2, TArgument3>(LogLevel level, IFormatProvider formatProvider, [Localizable(false)] string message, TArgument1 argument1, TArgument2 argument2, TArgument3 argument3)
{
if (IsEnabled(level))
{
WriteToTargets(level, formatProvider, message, new object[] { argument1, argument2, argument3 });
WriteToTargets(level, formatProvider, message, new object[] { argument1, argument2, argument3 });
}
}

Expand All @@ -434,7 +439,7 @@ public void Log<TArgument>(LogLevel level, [Localizable(false)] string message,
/// <param name="argument3">The third argument to format.</param>
[MessageTemplateFormatMethod("message")]
public void Log<TArgument1, TArgument2, TArgument3>(LogLevel level, [Localizable(false)] string message, TArgument1 argument1, TArgument2 argument2, TArgument3 argument3)
{
{
if (IsEnabled(level))
{
WriteToTargets(level, message, new object[] { argument1, argument2, argument3 });
Expand Down