Modules: Concurrency issue with module context fix

[pm-nikhil-vaidya]

#4239

[scr-oath]

Please see #4631 as an alternative approach, which uses the read-lock when calling the parallel bidder hook stages.

NOTE: I wrote it with Claude code and need to review myself - it may not be exactly what I wanted - I may want it to have better control over the lock than it currently does.

I just walked through it - it seems fine - each of the hookExecutor methods seems to call saveModuleContexts AFTER calling the executeStage and therefore holding a single lock for the duration of the executeGroup is fine - I might have grabbed and held at each hook invocation, but conceptually you can't return until all hooks are called so it's perfectly fine to only have one read lock held, so 👍

[scr-oath]

Ok - so taking another look - I think that *sync.Map would be a better option - I believe that models Java's ConcurrentHashMap where it tries to do things without locking if possible but resorts to locking in sticky situations. You're basically inventing sync.Map without that faster path here by forcing users to use Get/Set where they could be using Load/Store to accomplish the same effect and without reinventing any wheels.

CORRECTION from gemini: Implementing something with lock around map is probably best/fastest apparently.

Go 1.24+ Update: The standard Go map implementation was upgraded to Swiss Tables (a high-performance hash table design by Google). This makes the standard map + RWMutex significantly faster for lookups and inserts, further widening the gap between it and sync.Map for general use.

I'll have to take another look at this when I'm fresh. Could be ready to go 🤞

[bsardo]

@postindustria-code can you please review?

[scr-oath]

@pm-nikhil-vaidya - Happy New Year - can you take a look at the diff and see if you are comfortable applying it?
#4603 (comment)

[pm-nikhil-vaidya]

@scr-oath I have implemented the solution you suggested as it removes intermediate copy while copying the module context and removes extra lock mechanism while reading. I have just added a check in the Insert() function in case of map map is empty.

func (mc *ModuleContext) Insert(seq iter.Seq2[string, any]) {
	if mc == nil {
		return
	}

	mc.Lock()
	defer mc.Unlock()

	if mc.data == nil {
		mc.data = make(map[string]any)
	}

	maps.Insert(mc.data, seq)
}

You can review and let me know if any more changes required

[legendko]

I understand the motivation behind introducing All() for performance reasons. That makes sense, especially given how hot this path can be. However I see a possible deadlock issue with its usage.

ModuleContext.All() holds an RLock for the entire duration of iteration and executes caller-provided code while that lock is held. This creates a structural deadlock hazard:

• If code inside the iteration attempts to call Set(), Insert(), or any write operation → guaranteed deadlock
• Multiple concurrent bidders might trigger this without any module author error
• Plus if iteration is slow or blocks → writer starvation for all concurrent bidders

F.i. this pattern is very natural and currently deadlocks:

for k, v := range mc.All() {
    if shouldUpdate(k, v) {
        mc.Set(k, newValue) // deadlocks: tries to upgrade RLock → Lock
    }
}

GetAll() seems to be safer to use although being a bit slower and using copies

@pm-nikhil-vaidya @scr-oath what are you thoughts on this?

[postindustria-code]

Quick follow-up / clarification:

The previous comment was also from me (@legendko) — just posted from my personal GitHub account by mistake. Apologies for any confusion; I wanted to continue the discussion here from this account.

While we’re on the topic, I’d like to raise one additional concern, since this is a breaking API change for module authors:

• Should this be explicitly treated as a breaking change (e.g. version bump / release notes)?
• A migration guide for external modules? This exact issue arised from a custom module

[pm-nikhil-vaidya]

@bsardo @scr-oath @postindustria-code

I’ve rolled back to the previous implementation. I’ve also retained the approach suggested by @scr-oath for cases where a user prefers performance over the potential risk of a deadlock.

[scr-oath]

suggestion: Make with the right size for copy if nil.

Suggested change

	mc.data = make(map[string]any)
	mc.data = make(map[string]any, len(data))

[pm-nikhil-vaidya]

Done

[scr-oath]

suggestion: remove this

I thought we had decided against exposing All due to its risk of deadlock if held in reverse order with another

[pm-nikhil-vaidya]

Done

[scr-oath]

suggestion: remove this

I thought we decided against exporing Insert also for deadlock concerns.

[pm-nikhil-vaidya]

Done

[bsardo]

@pm-nikhil-vaidya we discussed @scr-oath's comments today. Let's remove Insert and All as he suggested and then we should be good.

[scr-oath]

All three previous change requests have been addressed:

1. ✅ make(map[string]any, len(data)) — properly sized allocation in SetAll when data is nil
2. ✅ All() iterator removed — eliminates deadlock risk from holding lock across iteration
3. ✅ Insert() removed — same concern addressed

Concurrency analysis of put(): The copy-based approach (existingCtx.SetAll(mCtx.GetAll())) is safe. Go evaluates mCtx.GetAll() first (acquires/releases mCtx.RLock), then passes the returned plain map to existingCtx.SetAll() (acquires/releases existingCtx.Lock). No two ModuleContext locks are ever held simultaneously. No reverse-order deadlock is possible since ModuleContext methods never reference back to moduleContexts.

nitpick (non-blocking): SetAll comment says "replaces all data" but maps.Copy merges into the existing map (existing keys not in data are preserved). The merge behavior is correct for this use case (accumulating context across stages), but the comment could say "merges data into the context" to be precise.

LGTM — the RWMutex + copy-based approach is sound, modules are all updated correctly, and tests are comprehensive.