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.

Sign up for GitHub

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

Jump to bottom

storage: storage.(*Writer).Write() with cancelled context causes write race #6816

Closed
dragonsinth opened this issue Oct 6, 2022 · 1 comment
Closed

storage: storage.(*Writer).Write() with cancelled context causes write race #6816

dragonsinth opened this issue Oct 6, 2022 · 1 comment
Assignees
@tritone
Labels
api: storage Issues related to the Cloud Storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@dragonsinth
Copy link
Contributor

dragonsinth commented Oct 6, 2022 •
edited

Client

Google Cloud Storage

Code

run the following test with -race enabled

package gcs_test

import (
	"cloud.google.com/go/storage"
	"context"
	"testing"
)

func TestWriteRace(t *testing.T) {
	ctx, cancel := context.WithCancel(context.Background())
	cancel()
	cl, err := storage.NewClient(ctx)
	t.Log(err)
	writer := cl.Bucket("foo").Object("bar").NewWriter(ctx)
	_, err = writer.Write([]byte("data"))
	t.Log(err)
	err = writer.Close()
	t.Log(err)
}

Expected behavior

Either Write or Close fails with context.Cancelled

Actual behavior

==================
WARNING: DATA RACE
Write at 0x00c0035d9880 by goroutine 58:
  cloud.google.com/go/storage.(*Writer).openWriter()
      go/pkg/mod/cloud.google.com/go/storage@v1.26.0/writer.go:198 +0xa14
  cloud.google.com/go/storage.(*Writer).Write()
      go/pkg/mod/cloud.google.com/go/storage@v1.26.0/writer.go:129 +0x94
  gcs_test.TestWriteRace()
      ./race_test.go:15 +0x534
  testing.tRunner()
      GOROOT/src/testing/testing.go:1446 +0x188
  testing.(*T).Run.func1()
      GOROOT/src/testing/testing.go:1493 +0x40

Previous read at 0x00c0035d9880 by goroutine 59:
  cloud.google.com/go/storage.(*Writer).CloseWithError()
      go/pkg/mod/cloud.google.com/go/storage@v1.26.0/writer.go:225 +0x148
  cloud.google.com/go/storage.(*Writer).monitorCancel()
      go/pkg/mod/cloud.google.com/go/storage@v1.26.0/writer.go:215 +0x13c
  cloud.google.com/go/storage.(*Writer).openWriter.func2()
      go/pkg/mod/cloud.google.com/go/storage@v1.26.0/writer.go:179 +0x34

Goroutine 58 (running) created at:
  testing.(*T).Run()
      GOROOT/src/testing/testing.go:1493 +0x55c
  testing.runTests.func1()
      GOROOT/src/testing/testing.go:1846 +0x90
  testing.tRunner()
      GOROOT/src/testing/testing.go:1446 +0x188
  testing.runTests()
      GOROOT/src/testing/testing.go:1844 +0x6c0
  testing.(*M).Run()
      GOROOT/src/testing/testing.go:1726 +0x870
  gcs_test.TestMain.func1()
      ./setup_test.go:15 +0x58
  gcs_test.TestMain()
      ./setup_test.go:16 +0x28
  main.main()
      _testmain.go:73 +0x300

Goroutine 59 (finished) created at:
  cloud.google.com/go/storage.(*Writer).openWriter()
      go/pkg/mod/cloud.google.com/go/storage@v1.26.0/writer.go:179 +0x478
  cloud.google.com/go/storage.(*Writer).Write()
      go/pkg/mod/cloud.google.com/go/storage@v1.26.0/writer.go:129 +0x94
  gcs_test.TestWriteRace()
      ./race_test.go:15 +0x534
  testing.tRunner()
      GOROOT/src/testing/testing.go:1446 +0x188
  testing.(*T).Run.func1()
      GOROOT/src/testing/testing.go:1493 +0x40
==================

Additional context

e.g. Started after upgrading to v1.26.0.
@dragonsinth dragonsinth added the triage me I really want to be triaged. label Oct 6, 2022
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Oct 6, 2022
@dragonsinth dragonsinth mentioned this issue Oct 6, 2022
Merged
@dragonsinth
Copy link
Contributor Author

#6817

@sydney-munro sydney-munro added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Oct 10, 2022
@andrewsg andrewsg added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Oct 13, 2022
tritone added a commit to tritone/google-cloud-go that referenced this issue Oct 13, 2022
@tritone
test(storage): add test for Writer race
612a695 
Adds a test to detect a potential race condition that can arise
if the context is cancelled before writing to GCS. We already
have a test that detects a similar condition after we have
already started writing the object, but this test looks at the context
being cancelled before any writes occur.

When run with -race, this test fails in storage/v1.27.0, but it
passes when run with the fix in googleapis#6817.

Updates googleapis#6816
@tritone tritone mentioned this issue Oct 13, 2022
Merged
tritone added a commit that referenced this issue Oct 17, 2022
@tritone
test(storage): add test for Writer race (#6862)
f968297 
Adds a test to detect a potential race condition that can arise
if the context is cancelled before writing to GCS. We already
have a test that detects a similar condition after we have
already started writing the object, but this test looks at the context
being cancelled before any writes occur.

When run with -race, this test fails in storage/v1.27.0, but it
passes when run with the fix in #6817.

Updates #6816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tritone tritone

Labels
api: storage Issues related to the Cloud Storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@andrewsg @dragonsinth @tritone @sydney-munro