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

Drop go.uber.org/atomic in favor of sync/atomic #3443

Merged
merged 3 commits into from
Nov 1, 2024
Merged

Drop go.uber.org/atomic in favor of sync/atomic #3443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42d29e3
Drop go.uber.org/atomic in favor of sync/atomic
stefanvanburen Oct 31, 2024
2515c42
Add back pointer
stefanvanburen Nov 1, 2024
1a41b94
Be more defensive above checking the atomicValue type
stefanvanburen Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ require (
github.com/tetratelabs/wazero v1.8.1
go.lsp.dev/jsonrpc2 v0.10.0
go.lsp.dev/protocol v0.12.0
go.uber.org/atomic v1.11.0
go.uber.org/zap v1.27.0
go.uber.org/zap/exp v0.3.0
golang.org/x/crypto v0.28.0
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,6 @@ go.opentelemetry.io/otel/trace v1.31.0 h1:ffjsj1aRouKewfr85U2aGagJ46+MvodynlQ1HY
go.opentelemetry.io/otel/trace v1.31.0/go.mod h1:TXZkRk7SM2ZQLtR6eoAWQFIHPvzQ06FJAsO1tJg480A=
go.opentelemetry.io/proto/otlp v1.0.0 h1:T0TX0tmXU8a3CbNXzEKGeU5mIVOdf0oykP+u2lIVU/I=
go.opentelemetry.io/proto/otlp v1.0.0/go.mod h1:Sy6pihPLfYHkr3NkUbEhGHFhINUSI/v80hjKIs5JXpM=
go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE=
go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0=
go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
go.uber.org/mock v0.5.0 h1:KAMbZvZPyBPWgD14IrIQ38QCyjwpvVVV6K/bHl1IwQU=
Expand Down
2 changes: 1 addition & 1 deletion private/buf/bufcurl/verbose_transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ import (
"runtime"
"sort"
"strings"
"sync/atomic"

"connectrpc.com/connect"
"github.com/bufbuild/buf/private/pkg/verbose"
"go.uber.org/atomic"
)

type skipUploadFinishedMessageKey struct{}
Expand Down
9 changes: 4 additions & 5 deletions private/pkg/storage/limit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ package storage

import (
"context"

"go.uber.org/atomic"
"sync/atomic"
)

// LimitWriteBucket returns a [WriteBucket] that writes to [writeBucket]
Expand All @@ -42,7 +41,7 @@ type limitedWriteBucket struct {
func newLimitedWriteBucket(bucket WriteBucket, limit int64) *limitedWriteBucket {
return &limitedWriteBucket{
WriteBucket: bucket,
currentSize: atomic.NewInt64(0),
stefanvanburen marked this conversation as resolved.
Show resolved Hide resolved
currentSize: &atomic.Int64{},
limit: limit,
}
}
Expand Down Expand Up @@ -78,15 +77,15 @@ func (o *limitedWriteObjectCloser) Write(p []byte) (int, error) {
writeSize := int64(len(p))
newBucketSize := o.bucketSize.Add(writeSize)
if newBucketSize > o.limit {
o.bucketSize.Sub(writeSize)
o.bucketSize.Add(-writeSize)
return 0, &errWriteLimitReached{
Limit: o.limit,
ExceedingBy: newBucketSize - o.limit,
}
}
writtenSize, err := o.WriteObjectCloser.Write(p)
if int64(writtenSize) < writeSize {
o.bucketSize.Sub(writeSize - int64(writtenSize))
o.bucketSize.Add(-(writeSize - int64(writtenSize)))
}
return writtenSize, err
}
30 changes: 27 additions & 3 deletions private/pkg/storage/storageos/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ import (
"io/fs"
"os"
"path/filepath"
"sync/atomic"

"github.com/bufbuild/buf/private/pkg/filepathext"
"github.com/bufbuild/buf/private/pkg/normalpath"
"github.com/bufbuild/buf/private/pkg/storage"
"github.com/bufbuild/buf/private/pkg/storage/storageutil"
"go.uber.org/atomic"
"github.com/bufbuild/buf/private/pkg/syserror"
)

// errNotDir is the error returned if a path is not a directory.
Expand Down Expand Up @@ -359,7 +360,7 @@ type writeObjectCloser struct {
path string
// writeErr contains the first non-nil error caught by a call to Write.
// This is returned in Close for atomic writes to prevent writing an incomplete file.
writeErr atomic.Error
writeErr onceError
}

func newWriteObjectCloser(
Expand All @@ -375,7 +376,7 @@ func newWriteObjectCloser(
func (w *writeObjectCloser) Write(p []byte) (int, error) {
n, err := w.file.Write(p)
if err != nil {
w.writeErr.CompareAndSwap(nil, err)
w.writeErr.Store(err)
}
return n, toStorageError(err)
}
Expand Down Expand Up @@ -404,6 +405,29 @@ func (w *writeObjectCloser) Close() error {
return err
}

// onceError is an object that will only store an error once.
type onceError struct {
err atomic.Value
}

// Store stores the err.
func (e *onceError) Store(err error) {
e.err.CompareAndSwap(nil, err)
}

// Load loads the stored error.
func (e *onceError) Load() error {
atomicValue := e.err.Load()
if atomicValue == nil {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a really bad situation - I need to review my Golang a bit, but if this is non-nil, this means that the thing that was stored was not an error on e.err, which would be a system error in itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think for safety purposes you could move onceError to a different package so someone within this package can't muck with the e.err value directly (instead of using Store)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but that's not really what I'm saying - I'm saying that you are returning nil when ok == false, and ok should only be false if e.err.Load() is not of type error, which would be a really bad scenario (somehow something other than an error got stored on e.err. ok might also be falseife.err.Load()isnil`, but this is where my Golang is failing me - I assume that's the only thing that is meant to be captured here.

I would imagine that if you want to do something like this, you need to do:

errIface := e.err.Load()
if errIface == nil {
  return nil
}
err, ok := errIface.(error) 
if !ok {
  return syserror.Newf("expected error but got %T", errIface)
}
return err

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that looks more technically correct to me - if you somehow stored something that isn't type error, you'd always just get nil currently. I'll update to your approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1a41b94 - feel free to change, just preferred the atomicValue var name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - I would probably update bufstream as well

}
err, ok := atomicValue.(error)
if !ok {
return syserror.Newf("expected error but got %T", atomicValue)
}
return err
}

// newErrNotDir returns a new Error for a path not being a directory.
func newErrNotDir(path string) *normalpath.Error {
return normalpath.NewError(path, errNotDir)
Expand Down
2 changes: 1 addition & 1 deletion private/pkg/storage/storagetesting/storagetesting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"sort"
"strconv"
"sync"
"sync/atomic"
"testing"

"github.com/bufbuild/buf/private/pkg/slicesext"
Expand All @@ -38,7 +39,6 @@ import (
"github.com/bufbuild/buf/private/pkg/tmp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/atomic"
)

const (
Expand Down
6 changes: 3 additions & 3 deletions private/pkg/thread/thread_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ package thread

import (
"context"
"sync/atomic"
"testing"

"github.com/stretchr/testify/assert"
"go.uber.org/atomic"
)

func TestParallelizeWithImmediateCancellation(t *testing.T) {
Expand All @@ -35,7 +35,7 @@ func TestParallelizeWithImmediateCancellation(t *testing.T) {
)
for i := 0; i < jobsToExecute; i++ {
jobs = append(jobs, func(_ context.Context) error {
executed.Inc()
executed.Add(1)
return nil
})
}
Expand All @@ -49,7 +49,7 @@ func TestParallelizeWithImmediateCancellation(t *testing.T) {
var jobs []func(context.Context) error
for i := 0; i < 10; i++ {
jobs = append(jobs, func(_ context.Context) error {
executed.Inc()
executed.Add(1)
return nil
})
}
Expand Down