pop
pop copied to clipboard
Proposal: remove side-effects of `Eager`
Description
I am getting some reports from go test -race when using parallel tests:
WARNING: DATA RACE
Write at 0x00c000596b88 by goroutine 61:
github.com/gobuffalo/pop/v6.(*Connection).disableEager()
/home/patrik/git/go/pkg/mod/github.com/gobuffalo/pop/[email protected]/connection.go:239 +0xbe
github.com/gobuffalo/pop/v6.(*Connection).Create()
/home/patrik/git/go/pkg/mod/github.com/gobuffalo/pop/[email protected]/executors.go:178 +0x95
When looking a bit into the code, I was actually surprised to see that Eager is implemented as a side-effect of the connection and query, although it is reset after each execution. Is there a specific reason for this? Using side-effects here makes it impossible to use connections and queries concurrently.
I know there are other problems with concurrency, namely with the mysql driver #530. I also found related API problems with how eager is currently implemented #307
In my opinion, we should either
- decide that connections and queries don't support concurrency
- fix concurrency issues
Side-effects are always a problem with concurrency, so refactoring eager mode to not use side-effects is probably the best way to go.
Steps to Reproduce the Problem
Call c.Create in multiple parallel go routines.
Expected Behavior
Create multiple models concurrently.
Actual Behavior
Race-condition as eager uses side-effects.
After a bit of research the data race seems to be (kind of) a false positive. The function (*Connection).Eager returns a shallow copy of the connection, therefore there should be no issue with parallel usage of the connection.
However, a user might think "Oh, I always want to use eager mode, so I just call Eager on the connection after creation, and it works" which does not work, and produces a real data race.
I would suggest to still remove all side-effects from eager mode.