Nondeterminism when incrementing a variable concurrently

[andychu]

I did a little more testing based on the lobsters discussion: https://lobste.rs/s/hj3np3/mvdan_sh_posix_shell_go

If I run f | f with the function f below (using the elvish-HEAD binary), sometimes the variable is incremented by one, and sometimes it's incremented by 2.

My suspicion is that this is a race condition. If that's the case I think it would be undefined behavior in Go, and would be flagged by Go's race detector (though I didn't try).

~/install> fn f { echo ---;  a = (+ $a 1) }


~/install> echo $a
22
~/install> 
~/install> f|f
---
~/install> echo $a
23
~/install> f|f
---
~/install> echo $a
25
~/install> f|f
---
~/install> echo $a
26
~/install> f|f
---
~/install> echo $a
27
~/install> f|f
---
~/install> echo $a
29

[xiaq]

It is a race condition, but the underlying variable is mutex-guarded:

elvish/pkg/eval/vars/ptr.go

Lines 40 to 52 in bd70489

	// GetRaw returns the value pointed by the pointer without any conversion.
	func (v PtrVar) GetRaw() interface{} {
	v.mutex.RLock()
	defer v.mutex.RUnlock()
	return reflect.Indirect(reflect.ValueOf(v.ptr)).Interface()
	}
	// Get sets the value pointed by the pointer, after conversion using ScanToGo.
	func (v PtrVar) Set(val interface{}) error {
	v.mutex.Lock()
	defer v.mutex.Unlock()
	return vals.ScanToGo(val, v.ptr)
	}

Go's race condition detector is only supposed to catch race conditions that can cause memory corruption, so it won't catch this one.

Elvish doesn't have a good story for memory sharing yet. A GIL is not a good solution; most of the interpreter state is actually concurrency-safe anyway. I would like to develop a set of synchronization primitives later.

[andychu]

BTW I also get this behavior for lists and maps:

~/install> fn chlist { echo ---; mylist[0] = x }
~/install> mylist = []
~/install> chlist
---
Exception: variable $mylist does not exist, compiler bug
Traceback:
  [tty], line 1:
    fn chlist { echo ---; mylist[0] = x }
  [tty], line 1:
    chlist
~/install> 

[xiaq]

That's because Elvish's variables require declaration before use. This is fine:

~> mylist = [a]
~> fn chlist { echo ---; mylist[0] = x }
~> chlist
---

Still, Elvish should have caught that when chlist is not defined during the compilation phase. I have filed #889 for this.

[krader1961]

Expanding on @xiaq's answer: The race is in the expansion of $a and the assignment to a. The statement a = (+ $a 1) can expand $a in both go routines before the assignment that the other thread makes can be seen. Which causes both go routines to increment and assign the same value. The use of a mutex when accessing or modifying the var, in isolation, is insufficient. Concurrent modification of global vars is inherently problematic (i.e., race prone) without using an explicit mutex guarding that global var.

[xiaq]

See #1003 for a proposed atomic mutation primitive.