Skip to content

Fix the TestConcurrent test to pass Go's race detection. #166

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

Merged
merged 2 commits into from
Nov 13, 2013

Conversation

thejerf
Copy link
Contributor

@thejerf thejerf commented Nov 8, 2013

Running the race detection against the previous version of this test reveals
race conditions surrounding the "max" variable. Subsequent work in this patch
also revealed race conditions surrounding canStop.

Additionally, it is not valid to call *Testing.FailNow in goroutines started
by the tests.

This should retain the meaning of the original test while cleaning up the race
conditions and guaranteeing the *Testing.Fatal call occurs in the correct
goroutine.

It also forces each connection to successfully SELECT 1 at least once. In
previous versions of this test it is entirely possible for canStop to be set to
true before any queries have been run.

Running the race detection against the previous version of this test reveals
race conditions surrounding the "max" variable. Subsequent work in this patch
also revealed race conditions surrounding canStop.

Additionally, it is not valid to call *Testing.FailNow in goroutines started
by the tests.

This should retain the meaning of the original test while cleaning up the race
conditions and guaranteeing the *Testing.Fatal call occurs in the correct
goroutine.
@julienschmidt
Copy link
Member

A few things:

  • max is indeed not concurrency safe
  • You are right with FailNow, too
  • the canStoprace condition is completely fine and intended. We don't care if canStop is set multiple times and overwritten, since it is always true afterwards
  • SELECT 1(should be changed to DO 1) is only in there to keep the connection busy. db.Beginalready executes a query on the connection (START TRANSACTION)

@thejerf
Copy link
Contributor Author

thejerf commented Nov 9, 2013

I should perhaps explain why I submitted this in the first place. I was examining which MySQL driver to use, and was using test coverage and race detection as part of my decision. You really don't want the race detector to say anything, because it makes the driver look bad.

Fortunately, I kept digging in and figured out what was really going on. (Partially influenced by the fact that I actually had a very similar case in one of my own test suites, of a mostly-harmless race condition in some concurrency testing that didn't exist in the code itself.)

In fact canStop is still "really" a race condition in my patch at the semantic level; all I've really done is "shut up" the race detector spuriously. However, I'd submit that that is still a feature, in terms of not triggering the race detector on something you don't want to be a race condition when people run the tests. It is, unapologetically, a matter of appearances. It is rather a pity that it requires so many lines of code to accomplish that. (You can golf it down, but it seemed to me like it got "obfuscated" pretty quickly.)

I made it do at least one SELECT 1 as just a sort of paranoia test. I like paranoia in my tests. I'll happily remove that bit if you like.

@julienschmidt
Copy link
Member

Here is my try to silence the race detector with a bit less clunky test:

func TestConcurrent(t *testing.T) {
    if enabled, _ := readBool(os.Getenv("MYSQL_TEST_CONCURRENT")); !enabled {
        t.Skip("MYSQL_TEST_CONCURRENT env var not set")
    }

    runTests(t, dsn, func(dbt *DBTest) {
        var max int
        err := dbt.db.QueryRow("SELECT @@max_connections").Scan(&max)
        if err != nil {
            dbt.Fatalf("%s", err.Error())
        }
        dbt.Logf("Testing up to %d concurrent connections \r\n", max)

        var remaining, succeeded int32 = int32(max), 0

        var wg sync.WaitGroup
        wg.Add(max)

        var fatalError string
        var once sync.Once
        fatal := func(s string, vals ...interface{}) {
            once.Do(func() {
                fatalError = fmt.Sprintf(s, vals...)
            })
        }

        for i := 0; i < max; i++ {
            go func(id int) {
                defer wg.Done()

                tx, err := dbt.db.Begin()
                atomic.AddInt32(&remaining, -1)

                if err != nil {
                    if err.Error() != "Error 1040: Too many connections" {
                        fatal("Error on Conn %d: %s", id, err.Error())
                    }
                    return
                }

                // keep the connection busy until all connections are open
                for remaining > 0 {
                    if _, err = tx.Exec("DO 1"); err != nil {
                        fatal("Error on Conn %d: %s", id, err.Error())
                        return
                    }
                }

                if err = tx.Commit(); err != nil {
                    fatal("Error on Conn %d: %s", id, err.Error())
                    return
                }

                // everything went fine with this connection
                atomic.AddInt32(&succeeded, 1)
            }(i)
        }

        // wait until all conections are open
        wg.Wait()

        if fatalError != "" {
            dbt.Fatal(fatalError)
        }

        dbt.Logf("Reached %d concurrent connections \r\n", succeeded)
    })
}

@thejerf
Copy link
Contributor Author

thejerf commented Nov 9, 2013

Looks like an improvement. I have no opinion on how the paperwork gets done, so you can close this pull and commit that if it's easier.

@julienschmidt
Copy link
Member

No, I hoped you would adopt some of the suggestions in your PRs update 😉

@thejerf
Copy link
Contributor Author

thejerf commented Nov 9, 2013

OK, no problem. :) It will probably be next Tuesday, I've got travel.

@julienschmidt
Copy link
Member

LGTM

julienschmidt added a commit that referenced this pull request Nov 13, 2013
Fix the TestConcurrent test to pass Go's race detection.
@julienschmidt julienschmidt merged commit 587def8 into go-sql-driver:master Nov 13, 2013
@julienschmidt julienschmidt added this to the v1.2 milestone Apr 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants