Skip to content

Commit d5fa476

Browse files
committed
fix(identity): avoid nil notifier dereference in LocalMembership.Close
Close() only returned early when identityDB.Notifier() failed with the expected storage.ErrNotSupported sentinel. For any other error it logged the failure and then called UnsubscribeAll() on the nil notifier anyway, panicking on the shutdown path — precisely when the storage backend is already unhealthy. Return for every error instead, logging the unexpected ones, and only unsubscribe when a notifier was actually obtained. Mirrors the handling already in subscribeNotifier. Adds a regression test that panics without this change. Fixes #2067 Signed-off-by: AkramBitar <akram@il.ibm.com>
1 parent 47ff1da commit d5fa476

3 files changed

Lines changed: 42 additions & 4 deletions

File tree

docs/services/identity.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,12 @@ core.TMSProvider.Update (token/core/tms.go)
222222
└── Role.Done() → LocalMembership.Close()
223223
```
224224

225+
`LocalMembership.Close()` is best-effort: it releases its key managers, then unsubscribes
226+
from the identity store's change notifier. If the store cannot supply a notifier — because
227+
it does not support one (`storage.ErrNotSupported`) or because it fails outright — the
228+
unsubscribe step is skipped and, in the failure case, logged. `Close()` returns no error and
229+
must never panic, since it runs on the shutdown path of an already-degraded node.
230+
225231
`role.Registry.Done()` closes wallets through a local `interface{ Close() }` assertion
226232
rather than through `driver.Wallet`, so wallet types with nothing to release need not
227233
implement a no-op `Close()`. If you add a wallet type that owns a goroutine, a ticker or

token/services/identity/membership/lm.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -276,11 +276,11 @@ func (l *LocalMembership) Close() {
276276

277277
notifier, err := l.identityDB.Notifier()
278278
if err != nil {
279-
if errors.Is(err, storage.ErrNotSupported) {
280-
// notithing to close
281-
return
279+
if !errors.Is(err, storage.ErrNotSupported) {
280+
logger.Errorf("failed to get identity notifier: [%s]", err)
282281
}
283-
logger.Errorf("failed to get identity notifier: [%s]", err)
282+
// no notifier, nothing to close
283+
return
284284
}
285285
if err := notifier.UnsubscribeAll(); err != nil {
286286
logger.Errorf("failed to unsubscribe [%s]: [%s]", l.IdentityType, err)

token/services/identity/membership/lm_discovery_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/LFDT-Panurus/panurus/token/services/identity/membership/mock"
2020
"github.com/LFDT-Panurus/panurus/token/services/logging"
2121
"github.com/LFDT-Panurus/panurus/token/services/storage"
22+
"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
2223
"github.com/stretchr/testify/assert"
2324
"github.com/stretchr/testify/require"
2425
)
@@ -392,3 +393,34 @@ func TestLocalMembership_Close(t *testing.T) {
392393
// Should be safe to call multiple times
393394
lm.Close()
394395
}
396+
397+
// TestLocalMembership_Close_NotifierError checks that Close does not panic when the
398+
// identity store fails to return a notifier with an error other than storage.ErrNotSupported.
399+
// In that case the notifier is nil and must not be used.
400+
func TestLocalMembership_Close_NotifierError(t *testing.T) {
401+
ip := &mock.IdentityProvider{}
402+
des := &mock.SignerDeserializerManager{}
403+
iss := &mock.IdentityStoreService{}
404+
iss.NotifierReturns(nil, errors.New("backend unavailable"))
405+
406+
kmp := &mock.KeyManagerProvider{}
407+
408+
lm := membership.NewLocalMembership(
409+
logging.MustGetLogger("test"),
410+
&mock.Config{},
411+
[]byte("netid"),
412+
des,
413+
iss,
414+
"testType",
415+
false,
416+
ip,
417+
kmp,
418+
)
419+
420+
// Load is not called on purpose: a failing Notifier makes Load fail, so Close must be
421+
// safe on its own. A nil notifier dereference would panic and fail the test.
422+
assert.NotPanics(t, lm.Close)
423+
424+
// Should be safe to call multiple times
425+
assert.NotPanics(t, lm.Close)
426+
}

0 commit comments

Comments
 (0)