address circular reference in LazyLoader with ThreadLocalProxy

[mattp-]

What does this PR do?

when lazyLoader was getting instantiated from inside already lazyloaded
modules (ie state), pillar/grain (and probably other) globals were
already populated. In this case they weren't getting dereferenced
properly when wrapped via the NamespacedDict wrapper, causing an
infinite loop. This should fix that scenario.

What issues does this PR fix or reference?

#50655 (comment)

pushing this to see what jenkins has to say

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Tests written?

Yes/No

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[terminalmage]

I think the whole reason was to make the context dicts thread-safe. @smarsching should probably have some input here.

[mattp-]

@thatch45 @terminalmage @smarsching figured out what was going on, pushed a proper fix. Would still appreciate a review but I'm fairly confident this fix is good for merge.

[terminalmage]

This does make more sense but I'd still appreciate eyes on this from @smarsching.

I'm also kind of worried about what other areas of Salt's code could potentially be affected by similar issues.

[waynew]

Minor style change.

I don't know enough about the ThreadLocalProxy to have a good opinion on what's going on with the rest of the change(s). I don't see anything that strikes me as obviously wrong, though.

[waynew]

You'll want to move this from import down with the rest of the from (somewhere below line 41).

[mattp-]

fixed

[smarsching]

I am sorry for being so so quiet about this issue, I am currently preoccupied with some other projects.

@mattp Basically, your changes look good to me. I only wondered, why you used

if isinstance(value, ThreadLocalProxy):
    value = ThreadLocalProxy.get_reference(value)

when

value =ThreadLocalProxy.unproxy(value)

is simpler and would achieve nearly the same thing (the only difference is that unproxy also handles the case where a proxy refers to another proxy that eventually refers to a non-proxy object).

On a more general note: I am not quite sure I understand why this issue exists in the first place and whether this is the best way to fix it. From the stack trace, it looks like there is a ThreadLocalProxy that refers to a NamespacedDictWrapper which in turn references the same ThreadLocalProxy.

As far as I understand the problem, this issue appears when a NamespacedDictWrapper is later modified to contain aThreadLocalProxy referencing the same NamespacedDictWrapper.

However, there is one thing, that I do not understand about this: Even without the ThreadLocalProxy, having a NamespacedDictWrapper referencing itself should lead to the same infinite loop, so I must be missing something...

[mattp-]

@smarsching no worries, wrt get_reference vs unproxy, it wasn't a conscious choice - I can update that.
wrt how this happens, I think the general problem is:

• modules.state is loaded, pillar proxy is injected
• modules.state creates a lazy loader, passes current pillar(proxy to existing context dict) as opts[pillar]
• that lazyloader doesnt deref, then namespacedict wraps it
• modules.state is reloaded via lazyloader
• since _inject_into_mod checks if the value is a ThreadLocalProxy, but not if it is a wrappeddict to a proxy, it is overridden
  ^ I think that is where the loop is created, because this dict wrapper is hiding the encapsulation to do the de-reference. The flow in this code is complicated to say the least, so hopefully my current understanding is correct.
  I've added a regression test and updated to use unproxy. thanks for the input 👍

[dwoz]

@waynew Are we good here?

Inaction