Improve handling of connection read errors #66
Conversation
|
@flowhamster Could you please take a look? I thought you might have some thoughts on this based on your recent changes in #57. |
|
As a before/after example, I wrote a simple Before this patch, I get an error which doesn't explain much: But with this patch applied I get much more helpful error message: |
|
ping @liggitt and @flowhamster |
|
+1, this looks like a good change to me. |
| @@ -330,6 +336,7 @@ func (l *Conn) processMessages() { | |||
| _, err := l.conn.Write(buf) | |||
| if err != nil { | |||
| l.Debug.Printf("Error Sending Message: %s", err.Error()) | |||
| message.Channel <- &PacketResponse{Error: fmt.Errorf("unable to send request: %s", err)} | |||
There was a problem hiding this comment.
seems like this channel could get double-notified with this change... should we do this as well?
close(message.Channel)
delete(l.chanResults, message.MessageID)
There was a problem hiding this comment.
or even wait to add it to l.chanResults until we've written successfully
There was a problem hiding this comment.
Thanks, I've added a close(message.Channel) line below and moved the line which adds the channel to l.chanResults from before writing the packet (above) to after (below).
If the reader() goroutine encounters an unexpected error when reading a packet a series of unwinding takes place: - The reader() goroutine shuts down. A deferred function runs which closes the connection. - Close() sends a MessageQuit message to the processMessages() goroutine. - The processMessages() goroutine shuts down. A deferred function runs which closes the results channels for any pending requests. - These pending request handlers receive a nil *PacketResponse because their response channel has been closed. They then return a not-very-helpful error string: "ldap: channel closed". This patch updates the reader() goroutine to set a closeErr value on the conn when it encounters an unexpected error reading a packet from the server. The processMessages() deferred function checks for this closeErr when it is shutting down due to the connection closing and sends this error in the *PacketResponse values to the pending request handlers *before* closing those results channels. This allows for the error which caused the shutdown to be bubbled up to all pending request calls. Docker-DCO-1.1-Signed-off-by: Josh Hawn <[email protected]> (github: jlhawn)
jlhawn
force-pushed
the
improve_read_error
branch
from
0639e91 to
fb58c8d
Compare
|
thanks! |
If the
reader()goroutine encounters an unexpected error when reading a packet a series of unwinding steps take place:reader()goroutine shuts down. A deferred function runs which closes the connection.Close()sends a MessageQuit message to theprocessMessages()goroutine.processMessages()goroutine shuts down. A deferred function runs which closes the results channels for any pending requests.*PacketResponsebecause their response channel has been closed. They then return a not-very-helpful error string:"ldap: channel closed".This patch updates the
reader()goroutine to set acloseErrvalue on the conn when it encounters an unexpected error reading a packet from the server. TheprocessMessages()deferred function checks forcloseErrwhen it is shutting down due to the connection closing and sends this error in the*PacketResponsevalues to the pending request handlers before closing those results channels. This allows for the error which caused the shutdown to be bubbled up to all pending request calls.