Skip to content

Commit

Permalink
Avoid acquiring shared lock recursively (#2076)
Browse files Browse the repository at this point in the history
Per the Go docs for sync.RWMutex, pending calls to RWMutex.Lock() block
calls to RWMutex.RLock(). With this understood we can see that the call
from getNamespace to getNamespaceByID acquires the read side of the
RWMutex twice - any interleaved call to RWMutex.Lock() between those two
RLock() calls will cause a deadlock - the Lock() call cannot continue
and the second RLock() call also will not continue.
  • Loading branch information
mmcshane authored Oct 20, 2021
1 parent 077d39c commit 6cbfa2a
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 1 deletion.
6 changes: 5 additions & 1 deletion common/namespace/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ func (r *registry) getNamespace(name string) (*Namespace, error) {
r.cacheLock.RLock()
defer r.cacheLock.RUnlock()
if id, ok := r.cacheNameToID.Get(name).(string); ok {
return r.getNamespaceByID(id)
return r.getNamespaceByIDLocked(id)
}
return nil, serviceerror.NewNotFound(
fmt.Sprintf("Namespace name %q not found", name))
Expand All @@ -483,6 +483,10 @@ func (r *registry) getNamespace(name string) (*Namespace, error) {
func (r *registry) getNamespaceByID(id string) (*Namespace, error) {
r.cacheLock.RLock()
defer r.cacheLock.RUnlock()
return r.getNamespaceByIDLocked(id)
}

func (r *registry) getNamespaceByIDLocked(id string) (*Namespace, error) {
if ns, ok := r.cacheByID.Get(id).(*Namespace); ok {
return ns, nil
}
Expand Down
26 changes: 26 additions & 0 deletions common/namespace/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -528,3 +528,29 @@ func (s *registrySuite) TestGetTriggerListAndUpdateCache_ConcurrentAccess() {
close(startChan)
waitGroup.Wait()
}

func TestCacheByName(t *testing.T) {
nsrec := persistence.GetNamespaceResponse{
Namespace: &persistencespb.NamespaceDetail{
Info: &persistencespb.NamespaceInfo{
Id: uuid.NewString(),
Name: "foo",
},
Config: &persistencespb.NamespaceConfig{},
ReplicationConfig: &persistencespb.NamespaceReplicationConfig{},
},
}
regPersist := persistence.NewMockMetadataManager(gomock.NewController(t))
regPersist.EXPECT().GetMetadata().Return(
&persistence.GetMetadataResponse{NotificationVersion: nsrec.NotificationVersion + 1}, nil)
regPersist.EXPECT().ListNamespaces(gomock.Any()).Return(&persistence.ListNamespacesResponse{
Namespaces: []*persistence.GetNamespaceResponse{&nsrec},
}, nil)
reg := namespace.NewRegistry(
regPersist, false, metrics.NewNoopMetricsClient(), log.NewNoopLogger())
reg.Start()
defer reg.Stop()
ns, err := reg.GetNamespace("foo")
require.NoError(t, err)
require.Equal(t, "foo", ns.Name())
}

0 comments on commit 6cbfa2a

Please sign in to comment.