Remove thunk mutex and use a signal channel and atomic pointer

[harrisonmetz]

This appears to fix fix #117 for me. Given I'm not that familiar with dataloaders I hope I didn't break anything with my simplification.

Rather than using a RWMutex (which needs to be locked 2 (and somethings 3 times)) in the thunk, this change makes it so that the result is stored in an atomic.Pointer[T] and the it being reader is signaled by closing of a done channel (chan struct{}). This way, "waiting" for the result to be ready is considered "durably blocked" by the synctest definition (https://pkg.go.dev/testing/synctest#hdr-Blocking). My hope is that this code is actually simpler (and faster) than what was there before and that now it works with synctest.

I ran the benchmarks. Before:

harry@Harrisons-Laptop dataloader % go test . -bench .
2025/09/28 21:55:43 avg: 0.000000
goos: darwin
goarch: arm64
pkg: github.com/graph-gophers/dataloader/v7
cpu: Apple M3
BenchmarkLoader-8       2025/09/28 21:55:43 avg: 0.000000
2025/09/28 21:55:43 avg: 0.000000
2025/09/28 21:55:44 avg: 32620.966667
2025/09/28 21:55:45 avg: 32846.180851
 2090329               560.6 ns/op
PASS
ok      github.com/graph-gophers/dataloader/v7  2.079s

After:

harry@Harrisons-Laptop dataloader % go test . -bench .
2025/09/28 21:56:41 avg: 0.000000
goos: darwin
goarch: arm64
pkg: github.com/graph-gophers/dataloader/v7
cpu: Apple M3
BenchmarkLoader-8       2025/09/28 21:56:41 avg: 0.000000
2025/09/28 21:56:41 avg: 0.000000
2025/09/28 21:56:42 avg: 34719.071429
2025/09/28 21:56:43 avg: 36589.312500
 2541984               498.5 ns/op
PASS
ok      github.com/graph-gophers/dataloader/v7  2.056s

[pavelnikolov]

Few requests:

• rename the channel variable
• add a nil check for the result; and
• add a test using testing/synctest

[pavelnikolov]

One more thing - the fix targets #117, but we don’t gain any protection against future regressions. Please add a focused test (ideally exercising testing/synctest) that fails on master and passes here.

[pavelnikolov]

result can be nil which would cause a panic. Please, add a nil check!

[harrisonmetz]

result can be nil which would cause a panic. Please, add a nil check!

Ok, I see, it can be nil if the user provided batch function did not set the items[i] element. Rather than check it each time the thunk is invoked, what if we always ensured that it was never nil.

In this loop:

for i, req := range reqs {
		req.result.Store(items[i])
		close(req.done)
	}

We could check item items[i] was nil and set to a item which an error that indicates that no value was set. What would you like the error to be? Should I just make one with fmt.Errorf() or should we declare one as a var ErrNoValueProvided = errors.New("no value provided") so the user could use errors.Is against it?

[pavelnikolov]

For starters, I'd be happy with just fmt.Errorf() - at the very least we remove the risk of a panic. Whether you add a specific error for that is up to you - no strong opinion here.

[pavelnikolov]

what if we always ensured that it was never nil

How do you imagine that? Can you provide an example, please?

[harrisonmetz]

I think the initial code also has would panic if results[i] of the batch function was nil.

dataloader/dataloader.go

Lines 264 to 272 in ab5318f

	result.value = v
	}
	result.mu.Unlock()
	}
	result.mu.RLock()
	defer result.mu.RUnlock()
	var ev *PanicErrorWrapper
	var es *SkipCacheError
	if result.value.Error != nil && (errors.As(result.value.Error, &ev) || errors.As(result.value.Error, &es)){

the code will check result.value.Error and result.value` could be nil.

I have made a separate to address this with a known error and updated the comment. This way if the batch function doesn't return a result for a given item, we can automatically give it an error result. (Similarly to how it does that if the length of the result isn't the same.)

efe1a00

[harrisonmetz]

One more thing - the fix targets #117, but we don’t gain any protection against future regressions. Please add a focused test (ideally exercising testing/synctest) that fails on master and passes here.

I could do that, but synctest is only available in go1.25 or higher. I think I'd have to bump the module version to that and then people couldn't use this in earlier versions of go. (Currently, I just bumped it to 1.19 for atomic.Pointer - which was only 1 more than 1.18 and still pretty old).

[pavelnikolov]

I see no problem with bumping the Go version to v1.25. In future, I'm planning to only support the last two minor Go versions. When a new release is cut it would not affect existing users of the previous version. And given that there are no breaking changes between Go v1.18 and v1.25 I see no reason why most of the users wouldn't be able to upgrade if they wanted to.

[harrisonmetz]

Here is the benchmark with my most recent changes (on a windows x86_64 computer):

PS C:\Users\harri\src\dataloader> go test . -bench .
2025/10/12 05:52:50 avg: 0.000000
goos: windows
goarch: amd64
pkg: github.com/graph-gophers/dataloader/v7
cpu: AMD Ryzen 7 5800X 8-Core Processor
BenchmarkLoader-16      2025/10/12 05:52:50 avg: 0.000000
2025/10/12 05:52:50 avg: 0.000000
2025/10/12 05:52:51 avg: 23074.604651
2025/10/12 05:52:52 avg: 26186.769231
 1716456               604.1 ns/op
PASS
ok      github.com/graph-gophers/dataloader/v7  2.129s

Here is it before:

PS C:\Users\harri\src\dataloader> go test . -bench .
2025/10/12 05:53:32 avg: 0.000000
goos: windows
goarch: amd64
pkg: github.com/graph-gophers/dataloader/v7
cpu: AMD Ryzen 7 5800X 8-Core Processor
BenchmarkLoader-16      2025/10/12 05:53:32 avg: 0.000000
2025/10/12 05:53:32 avg: 0.000000
2025/10/12 05:53:33 avg: 21831.260870
2025/10/12 05:53:34 avg: 24323.398058
 1496332               694.1 ns/op
PASS
ok      github.com/graph-gophers/dataloader/v7  2.281s

So the changes here both fix the synctest but also make it faster. It also is a net reduction in lines.

Regarding Go 1.25, I have a couple projects using DataLoaders and while one is on go 1.25 (and using synctest which is who I discovered this), the others are a bit earlier. I can't get upgrade to 1.25 but do what them to get this code as it's slightly faster. Would you be open to approving this PR on it's own merits without the synctest? (It's just a happy coincidence that it happens to fix that as well.)

[pavelnikolov]

Fair enough, upgrading Go version is outside of the scope of this PR.