From b36c982c7abc55196853c6052090ed6895508a8d Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Wed, 27 Sep 2023 13:32:42 -0700 Subject: [PATCH 1/3] randomWRR: attain lock before read in String() --- internal/wrr/random.go | 2 ++ internal/wrr/random_test.go | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 internal/wrr/random_test.go diff --git a/internal/wrr/random.go b/internal/wrr/random.go index 25bbd82594d6..3ffe921c1ec4 100644 --- a/internal/wrr/random.go +++ b/internal/wrr/random.go @@ -87,5 +87,7 @@ func (rw *randomWRR) Add(item any, weight int64) { } func (rw *randomWRR) String() string { + rw.mu.Lock() + defer rw.mu.Unlock() return fmt.Sprint(rw.items) } diff --git a/internal/wrr/random_test.go b/internal/wrr/random_test.go new file mode 100644 index 000000000000..1ca72e07ab24 --- /dev/null +++ b/internal/wrr/random_test.go @@ -0,0 +1,18 @@ +/* + * + * Copyright 2023 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package wrr From 269b3c09e28e385fd81943c833622ef8e57f4b83 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Fri, 6 Oct 2023 15:25:08 -0700 Subject: [PATCH 2/3] update random.go by removing mutex --- internal/wrr/random.go | 8 -------- internal/wrr/random_test.go | 18 ------------------ internal/wrr/wrr.go | 7 +++---- 3 files changed, 3 insertions(+), 30 deletions(-) delete mode 100644 internal/wrr/random_test.go diff --git a/internal/wrr/random.go b/internal/wrr/random.go index 3ffe921c1ec4..4d91fc6f580e 100644 --- a/internal/wrr/random.go +++ b/internal/wrr/random.go @@ -20,7 +20,6 @@ package wrr import ( "fmt" "sort" - "sync" "google.golang.org/grpc/internal/grpcrand" ) @@ -38,7 +37,6 @@ func (w *weightedItem) String() string { // randomWRR is a struct that contains weighted items implement weighted random algorithm. type randomWRR struct { - mu sync.RWMutex items []*weightedItem // Are all item's weights equal equalWeights bool @@ -52,8 +50,6 @@ func NewRandom() WRR { var grpcrandInt63n = grpcrand.Int63n func (rw *randomWRR) Next() (item any) { - rw.mu.RLock() - defer rw.mu.RUnlock() if len(rw.items) == 0 { return nil } @@ -72,8 +68,6 @@ func (rw *randomWRR) Next() (item any) { } func (rw *randomWRR) Add(item any, weight int64) { - rw.mu.Lock() - defer rw.mu.Unlock() accumulatedWeight := weight equalWeights := true if len(rw.items) > 0 { @@ -87,7 +81,5 @@ func (rw *randomWRR) Add(item any, weight int64) { } func (rw *randomWRR) String() string { - rw.mu.Lock() - defer rw.mu.Unlock() return fmt.Sprint(rw.items) } diff --git a/internal/wrr/random_test.go b/internal/wrr/random_test.go deleted file mode 100644 index 1ca72e07ab24..000000000000 --- a/internal/wrr/random_test.go +++ /dev/null @@ -1,18 +0,0 @@ -/* - * - * Copyright 2023 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package wrr diff --git a/internal/wrr/wrr.go b/internal/wrr/wrr.go index d0d82cf4f015..df866321a71a 100644 --- a/internal/wrr/wrr.go +++ b/internal/wrr/wrr.go @@ -21,12 +21,11 @@ package wrr // WRR defines an interface that implements weighted round robin. type WRR interface { - // Add adds an item with weight to the WRR set. - // - // Add and Next need to be thread safe. + // Add adds an item with weight to the WRR set. All Add must be only called + // before any calls to Next(). Add(item any, weight int64) // Next returns the next picked item. // - // Add and Next need to be thread safe. + // Next needs to be thread safe. Next() any } From 84f59a7792b4a1d5c3c6fb62763d09c4cbd6df25 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Fri, 6 Oct 2023 16:53:34 -0700 Subject: [PATCH 3/3] address comments --- internal/wrr/wrr.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/wrr/wrr.go b/internal/wrr/wrr.go index df866321a71a..91a40d7357e3 100644 --- a/internal/wrr/wrr.go +++ b/internal/wrr/wrr.go @@ -21,11 +21,12 @@ package wrr // WRR defines an interface that implements weighted round robin. type WRR interface { - // Add adds an item with weight to the WRR set. All Add must be only called - // before any calls to Next(). + // Add adds an item with weight to the WRR set. Add must be only called + // before any calls to Next. Add(item any, weight int64) // Next returns the next picked item. // - // Next needs to be thread safe. + // Next needs to be thread safe. Add may not be called after any call to + // Next. Next() any }