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

Race in easy_logging.go introduced in v1.8.0 #1059

Closed
bombsimon opened this issue Feb 23, 2024 · 1 comment · Fixed by #1060
Closed

Race in easy_logging.go introduced in v1.8.0 #1059

bombsimon opened this issue Feb 23, 2024 · 1 comment · Fixed by #1060
Assignees
Labels
bug Erroneous or unexpected behaviour status-pr_pending_merge A PR is made and is under review status-triage_done Initial triage done, will be further handled by the driver team

Comments

@bombsimon
Copy link
Contributor

  1. What version of GO driver are you using?

v1.8.0 (v1.7.2 works)

  1. What operating system and processor architecture are you using?

macOS 14.2.1, arm64

  1. What version of GO are you using?

1.21.3, 1.22.0

4.Server version:* E.g. 1.90.1

N/A

  1. What did you do?

Connected to Snowflake in more than one thread

  1. What did you expect to see?

No data to be shared across the instances. Potentially shared data should be thread safe.

  1. Can you set logging to DEBUG and collect the logs?

N/A

  1. What is your Snowflake account identifier, if any? (Optional)

N/A


Reproducible exampel

  • Ensure you're using v1.8.0 -> go get -u github.com/snowflakedb/gosnowflake@1.8.0
  • Run go test . -race with a main_test.go containing:
package main

import (
	"database/sql"
	"sync"
	"testing"

	"github.com/snowflakedb/gosnowflake"
)

func TestMain(t *testing.T) {
	cfg := &gosnowflake.Config{
		Account:          "X",
		User:             "X",
		Password:         "X",
	}

	dsn, err := gosnowflake.DSN(cfg)
	if err != nil {
		panic(err)
	}

	n := 10
	wg := sync.WaitGroup{}
	wg.Add(n)

	for range make([]int, n) {
		go func() {
			defer wg.Done()

			db, err := sql.Open("snowflake", dsn)
			if err != nil {
				panic(err)
			}

			_ = db.Ping()
		}()
	}

	wg.Wait()
}
Output
==================
WARNING: DATA RACE
Read at 0x0001042161e0 by goroutine 21:
  github.com/snowflakedb/gosnowflake.allowedToInitialize()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/easy_logging.go:120 +0x30
  github.com/snowflakedb/gosnowflake.initEasyLogging()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/easy_logging.go:41 +0x38
  github.com/snowflakedb/gosnowflake.buildSnowflakeConn()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/connection.go:760 +0x2bc
  github.com/snowflakedb/gosnowflake.SnowflakeDriver.OpenWithConfig()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/driver.go:38 +0x140
  github.com/snowflakedb/gosnowflake.SnowflakeDriver.Open()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/driver.go:26 +0x13c
  github.com/snowflakedb/gosnowflake.(*SnowflakeDriver).Open()
      <autogenerated>:1 +0x44
  database/sql.dsnConnector.Connect()
      /usr/local/go/src/database/sql/sql.go:758 +0x5c
  database/sql.(*dsnConnector).Connect()
      <autogenerated>:1 +0x20
  database/sql.(*DB).conn()
      /usr/local/go/src/database/sql/sql.go:1387 +0x76c
  database/sql.(*DB).PingContext.func1()
      /usr/local/go/src/database/sql/sql.go:850 +0x5c
  database/sql.(*DB).retry()
      /usr/local/go/src/database/sql/sql.go:1538 +0x4c
  database/sql.(*DB).PingContext()
      /usr/local/go/src/database/sql/sql.go:849 +0x7c
  database/sql.(*DB).Ping()
      /usr/local/go/src/database/sql/sql.go:867 +0x9c
  x.TestMain.func1()
      /Users/simon/tmp/test-go/racy-snowflake/main_test.go:36 +0x88

Previous write at 0x0001042161e0 by goroutine 27:
  github.com/snowflakedb/gosnowflake.(*initTrials).setInitTrial()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/easy_logging.go:26 +0x8ec
  github.com/snowflakedb/gosnowflake.initEasyLogging()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/easy_logging.go:53 +0x8e0
  github.com/snowflakedb/gosnowflake.buildSnowflakeConn()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/connection.go:760 +0x2bc
  github.com/snowflakedb/gosnowflake.SnowflakeDriver.OpenWithConfig()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/driver.go:38 +0x140
  github.com/snowflakedb/gosnowflake.SnowflakeDriver.Open()
      /Users/simon/go/pkg/mod/github.com/snowflakedb/gosnowflake@v1.8.0/driver.go:26 +0x13c
  github.com/snowflakedb/gosnowflake.(*SnowflakeDriver).Open()
      <autogenerated>:1 +0x44
  database/sql.dsnConnector.Connect()
      /usr/local/go/src/database/sql/sql.go:758 +0x5c
  database/sql.(*dsnConnector).Connect()
      <autogenerated>:1 +0x20
  database/sql.(*DB).conn()
      /usr/local/go/src/database/sql/sql.go:1387 +0x76c
  database/sql.(*DB).PingContext.func1()
      /usr/local/go/src/database/sql/sql.go:850 +0x5c
  database/sql.(*DB).retry()
      /usr/local/go/src/database/sql/sql.go:1538 +0x4c
  database/sql.(*DB).PingContext()
      /usr/local/go/src/database/sql/sql.go:849 +0x7c
  database/sql.(*DB).Ping()
      /usr/local/go/src/database/sql/sql.go:867 +0x9c
  x.TestMain.func1()
      /Users/simon/tmp/test-go/racy-snowflake/main_test.go:36 +0x88

Goroutine 21 (running) created at:
  x.TestMain()
      /Users/simon/tmp/test-go/racy-snowflake/main_test.go:28 +0xc4
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1648 +0x40

Goroutine 27 (running) created at:
  x.TestMain()
      /Users/simon/tmp/test-go/racy-snowflake/main_test.go:28 +0xc4
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1648 +0x40
==================

Root cause

The public shared variable here:

var easyLoggingInitTrials = initTrials{
everTriedToInitialize: false,
clientConfigFileInput: "",
configureCounter: 0,
}

Is read from without any mutex here:

triedToInitializeWithoutConfigFile := easyLoggingInitTrials.everTriedToInitialize && easyLoggingInitTrials.clientConfigFileInput == ""

Is written to without any mutex here:

i.everTriedToInitialize = true

Somewhat related to #616 and another reason to drop logging from the library and leave that to the caller.

@bombsimon bombsimon added the bug Erroneous or unexpected behaviour label Feb 23, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka self-assigned this Feb 23, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka added the status-in_progress Issue is worked on by the driver team label Feb 23, 2024
@sfc-gh-dszmolka
Copy link
Contributor

hi - thank you for raising this issue, all the details for reproduction and especially the solution is really appreciated! we'll review the PR

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-triage_done Initial triage done, will be further handled by the driver team label Feb 23, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka added status-pr_pending_merge A PR is made and is under review and removed status-in_progress Issue is worked on by the driver team labels Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Erroneous or unexpected behaviour status-pr_pending_merge A PR is made and is under review status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants