Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: remove side-effects of Eager #723

Open
zepatrik opened this issue May 27, 2022 · 1 comment
Open

Proposal: remove side-effects of Eager #723

zepatrik opened this issue May 27, 2022 · 1 comment
Labels
f: associations the associations feature in pop s: triage Some tests need to be run to confirm the issue

Comments

@zepatrik
Copy link
Contributor

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/v6@v6.0.1/connection.go:239 +0xbe
  github.com/gobuffalo/pop/v6.(*Connection).Create()
      /home/patrik/git/go/pkg/mod/github.com/gobuffalo/pop/v6@v6.0.1/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
Copy link
Contributor Author

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.

@sio4 sio4 added s: triage Some tests need to be run to confirm the issue f: associations the associations feature in pop labels Sep 20, 2022
@sio4 sio4 added this to the v6.1.0 milestone Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: associations the associations feature in pop s: triage Some tests need to be run to confirm the issue
Projects
None yet
Development

No branches or pull requests

2 participants