Skip to content

Commit

Permalink
peer: provide better debug for queued nil messages
Browse files Browse the repository at this point in the history
If the doneChan is nil, the nil message can be safely ignored.  However, this
is still a bug in the caller, so a warning with stacktrace is logged.

If the doneChan is non-nil, panicing is the only reasonable option to prevent
the program continuing on in an unknown state.

The peer code would already panic previously in wire.WriteMessageN for many
queued message types, and introducing a new panic call here should not
introduce any new crashes that would not have occurred before.
  • Loading branch information
jrick committed Nov 13, 2023
1 parent c077ac8 commit a623fe1
Showing 1 changed file with 17 additions and 0 deletions.
17 changes: 17 additions & 0 deletions peer/peer.go
Original file line number Diff line number Diff line change
Expand Up @@ -1688,6 +1688,23 @@ cleanup:
//
// This function is safe for concurrent access.
func (p *Peer) QueueMessage(msg wire.Message, doneChan chan<- struct{}) {
// Provide debug information when called with a nil message. This
// provides a more useful stack trace to callers than hitting the
// panic in a long-lived peer goroutine that contains no information
// about what caller queued the nil message.
if msg == nil {
// The semantics of whether a non-nil doneChan should be sent
// to or not, and the unknown program state this might lead
// to, doesn't leave a reasonable option to recover from this.
if doneChan != nil {
panic("peer: queued nil message")
}

log.Warnf("Attempt to send nil message type %T\nStack: %s",
msg, debug.Stack())

Check failure on line 1704 in peer/peer.go

View workflow job for this annotation

GitHub Actions / Go CI (1.20)

undefined: debug

Check failure on line 1704 in peer/peer.go

View workflow job for this annotation

GitHub Actions / Go CI (1.21)

undefined: debug
return
}

// Avoid risk of deadlock if goroutine already exited. The goroutine
// we will be sending to hangs around until it knows for a fact that
// it is marked as disconnected and *then* it drains the channels.
Expand Down

0 comments on commit a623fe1

Please sign in to comment.