-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
swarm: log unexpected listener errors #2277
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits, otherwise this lgtm.
p2p/net/swarm/swarm_listen.go
Outdated
@@ -127,6 +127,9 @@ func (s *Swarm) AddListenAddr(a ma.Multiaddr) error { | |||
for { | |||
c, err := list.Accept() | |||
if err != nil { | |||
if err != transport.ErrListenerClosed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.Is
?
@@ -152,8 +153,7 @@ func TestAcceptErrorGetCleanedUp(t *testing.T) { | |||
require.NoError(t, err) | |||
defer l.Close() | |||
_, err = l.Accept(context.Background()) | |||
require.EqualError(t, err, "accept goroutine finished") | |||
|
|||
require.Equal(t, err, transport.ErrListenerClosed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require.ErrorIs
?
p2p/transport/quicreuse/listener.go
Outdated
@@ -134,6 +136,9 @@ func (l *connListener) Run() error { | |||
for { | |||
conn, err := l.l.Accept(context.Background()) | |||
if err != nil { | |||
if err == quic.ErrServerClosed || strings.Contains(err.Error(), "use of closed network connection") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.Is
?
p2p/net/swarm/swarm_listen.go
Outdated
@@ -127,6 +127,9 @@ func (s *Swarm) AddListenAddr(a ma.Multiaddr) error { | |||
for { | |||
c, err := list.Accept() | |||
if err != nil { | |||
if err != transport.ErrListenerClosed { | |||
log.Error("swarm listener accept error: ", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this include which listener this is? For example:
log.Error("swarm listener accept error: ", err) | |
log.Errorf("swarm listener for %s accept error: %s", a, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using "%T" to print the type? I think it'll be more useful than trying to print the fields on some unknown struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What fields of an unknown struct? a
is the multiaddr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I misread this as log.Errorf("swarm listener %s accept error: %s", list, err)
. That makes sense.
Closes #2264