NLog variables are not threadsafe

[304NotModified]

@snakefoot I see your recommendation for gdc over var,

e.g.

If needing to modify global variables at runtime, then it is recommended to use the GDC layout renderer instead. Because NLog variables are not threadsafe, and should be seen as readonly variables. Also GDC will behave correctly if LoggingConfiguration should change (or reloaded).

Is there something we could do to make the current nlog variables threadsafe? A lot of users find the variables more natural to use.

[snakefoot]

Think people are cheated by old advice. Just like people activate throwexceptions=true. I guess the var-layout should never have been created since besides the pitfalls also gives a performance hit

[304NotModified]

Maybe,

But even we could give the nlog variables a GDC` under the hood? That would solve the issue?

[snakefoot]

NLog variables are special as they work as blobs in the NLog.config. Can contain an entire Layout config.

[snakefoot]

I guess if the IDictionary returned was similar to ConcurrentDictionary. Or a homemade synchronized Dictionary. Then also extra help where it doesn't return the same Layout-instance unless it is recognized as threadsafe. Or returns a homemade synchronized Layout that wraps the unsafe one. Lot of code to fix something where GDC already works. Would rather focus on making NLog work on CoreRt / NetNative by removing automatic assembly loading in NetCore, and instead introduce better fluent Config Api for integrating NLog extension targets.

[snakefoot]

I guess when support for JsonLayout is added (Or CsvLayout), then the Variables-dictionary (with SimpleLayout) will be made obsolete. See also #1312

Would prefer that the Variables remained pure-string-blobs (as they were originally). And one parsed the blob on every lookup (and performed recursive expansions of variables on every lookup). One could of course do caching so only doing the recursion expansion if variables had changed since last lookup.

Also funny how NLog expects that the insert order of variables is guaranteed stable. As NLog expects to expand variables in the order they are declared. But Dictionary / ConcurrentDictionary makes no promise about enumerating their items in the order they were inserted (Dictionary-class tries really hard to behave like it though). One should probably be careful with just replacing Dictionary with ConcurrentDictionary.

[304NotModified]

I guess when support for JsonLayout is added (Or CsvLayout), then the Variables-dictionary (with SimpleLayout) will be made obsolete. See also #1312

What do you mean? Should we do #1312, or should we skip that?

[snakefoot]

What do you mean? Should we do #1312, or should we skip that?

When implementing support for JsonLayout and CsvLayout in NLog-variables, then it will require new a interface. This new interface can then implement a threadsafe dictionary, that is backward compatible with the old one.

[304NotModified]

You mean, it's already a breaking change and so we could fix this structural?

[snakefoot]

You mean, it's already a breaking change and so we could fix this structural?

The new implementation that handles JsonLayout and CsvLayout will ofcourse introduce a new interface (that hopefully fixes all known issues). Then there is an exercise in trying to support the old existing interface to avoid any breaking changes.

Not sure I understand the meaning of fix this structural.

[304NotModified]

I was thinking, this advice, does it also count when not using layout renderers inside the value?

e.g.

<variable name="user" value="admin" />

[snakefoot]

Well I only wrote the advice for the ${var}-layoutrenderer wiki-page.

[snakefoot]

Btw. this issue has been closed without variables have become threadsafe to use (Only threadsafe to update)

[304NotModified]

Were here some specific steps we should do?

[snakefoot]

One could wrap Layouts that are not threadsafe by default in a special Layout that uses a lock-object.