pop icon indicating copy to clipboard operation
pop copied to clipboard

Proposal: remove side-effects of `Eager`

Open zepatrik opened this issue 3 years ago • 1 comments

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.

zepatrik avatar May 27 '22 16:05 zepatrik

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.

zepatrik avatar Jul 12 '22 15:07 zepatrik