Skip to content

Commit aff5c5c

Browse files
authored
Cleanup/deregister a mailbox if we fail to register (#172)
* Cleanup/deregister a mailbox if we fail to register * Fix the defers and mutex locking in mailboxes
1 parent 2768bfb commit aff5c5c

File tree

1 file changed

+28
-26
lines changed

1 file changed

+28
-26
lines changed

mailbox.go

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -121,40 +121,18 @@ func newMailbox(s *Server, name, nsName string, size int) (*Mailbox, error) {
121121
s.mu.Unlock()
122122

123123
s.mumb.Lock()
124-
defer s.mumb.Unlock()
125-
126124
_, ok := s.mailboxes[nsName]
127125
if ok {
128126
return nil, fmt.Errorf("%w: nsName=%s", ErrAlreadyRegistered, nsName)
129127
}
128+
s.mumb.Unlock()
130129

131-
timeout, cancel := context.WithTimeout(context.Background(), s.cfg.Timeout)
132-
err := s.registry.Register(timeout, nsName)
133-
cancel()
134-
// Check if the error is a particular fatal error
135-
// from etcd. Some errors have no recovery. See
136-
// the list of all possible errors here:
137-
//
138-
// https://github.com/etcd-io/etcd/blob/master/etcdserver/api/v3rpc/rpctypes/error.go
139-
//
140-
// They are unfortunately not classidied into
141-
// recoverable or non-recoverable.
142-
if err != nil && strings.Contains(err.Error(), "etcdserver: requested lease not found") {
143-
s.reportFatalError(err)
144-
return nil, err
145-
}
146-
if err != nil {
147-
return nil, err
148-
}
149-
150-
boxC := make(chan Request, size)
151130
cleanup := func() {
152131
s.mumb.Lock()
153-
defer s.mumb.Unlock()
154-
155132
// Immediately delete the subscription so that no one
156133
// can send to it, at least from this host.
157134
delete(s.mailboxes, nsName)
135+
s.mumb.Unlock()
158136

159137
// Deregister the name.
160138
var err error
@@ -165,13 +143,37 @@ func newMailbox(s *Server, name, nsName string, size int) (*Mailbox, error) {
165143
cancel()
166144
return err != nil
167145
})
168-
// Ingnore ErrNotOwner because most likely the previous owner panic'ed or exited badly.
146+
// Ingnore ErrNotOwner because most likely the previous owner panic'ed or exited badly.
169147
// So we'll ignore the error and let the mailbox creator retry later. We don't want to panic
170-
// in that case because it will take down more mailboxes and make it worse.
148+
// in that case because it will take down more mailboxes and make it worse.
171149
if err != nil && err != registry.ErrNotOwner {
172150
panic(fmt.Errorf("%w: unable to deregister mailbox: %v, error: %v", errDeregisteredFailed, nsName, err))
173151
}
174152
}
153+
154+
timeout, cancel := context.WithTimeout(context.Background(), s.cfg.Timeout)
155+
err := s.registry.Register(timeout, nsName)
156+
cancel()
157+
// Check if the error is a particular fatal error
158+
// from etcd. Some errors have no recovery. See
159+
// the list of all possible errors here:
160+
//
161+
// https://github.com/etcd-io/etcd/blob/master/etcdserver/api/v3rpc/rpctypes/error.go
162+
//
163+
// They are unfortunately not classidied into
164+
// recoverable or non-recoverable.
165+
if err != nil && strings.Contains(err.Error(), "etcdserver: requested lease not found") {
166+
s.reportFatalError(err)
167+
return nil, err
168+
}
169+
if err != nil {
170+
cleanup()
171+
return nil, err
172+
}
173+
174+
s.mumb.Lock()
175+
defer s.mumb.Unlock()
176+
boxC := make(chan Request, size)
175177
box := &Mailbox{
176178
name: name,
177179
nsName: nsName,

0 commit comments

Comments
 (0)