Fix crash when the server drops and does not return #19
Conversation
This adds a new test that closes the server and continue to write to it. It uncovered a bug with an unhandled nil pointer. It also uncovered some issues in the reconnection logic that were fixed at the same time.
|
|
|
This fixes moby downstream issue 35613 |
|
@mariussturm, @0cjs, @iporsut, @MiLk, can you review this bug fix? It fixes a problem that was missed when the server is goes completely offline. |
|
On it, but this is not trivial code. |
| func (r *TCPReader) listenUntilCloseSignal(signal chan string) { | ||
| defer func() { signal <- "done" }() | ||
| defer r.listener.Close() | ||
| func (r *TCPReader) accepter(connections chan net.Conn) { | ||
| for { | ||
| conn, err := r.listener.Accept() | ||
| if err != nil { | ||
| break |
There was a problem hiding this comment.
If an error occurs here, I think this will have no effect on the program but to silently stop the acceptor.
There was a problem hiding this comment.
This is the case, the thing with the whole tcpreader is to be a test harness, that is the reason all the functions are unexported. I should probably rename it so that it is clear that it is a test harness. It could probably be refactored and cleaned up to expose the API, but I am not aware of any downstream projects that require a GELF TCP Reader.
There was a problem hiding this comment.
Ah yes, I forgot about that.
It's fine then 👍
gelf/tcpreader.go
Outdated
| if sig == "stop" { | ||
| break | ||
| if len(conns) >= 1 { |
There was a problem hiding this comment.
Those 2 blocks being similar, could you refactor in a way which doesn't duplicate code?
|
This is the first time I hear about context, my go-fu is quite limited, but from what I read, I am not sure it would be cleaner than the multiple signals. The thing is that we need to be able to control the Am I missing something on the Context? |
| func (r *TCPReader) listenUntilCloseSignal(signal chan string) { | ||
| defer func() { signal <- "done" }() | ||
| defer r.listener.Close() | ||
| func (r *TCPReader) accepter(connections chan net.Conn) { | ||
| for { | ||
| conn, err := r.listener.Accept() | ||
| if err != nil { | ||
| break |
There was a problem hiding this comment.
Ah yes, I forgot about that.
It's fine then 👍
|
Thanks for the PR, LGTM! |
This adds a new test that closes the server and continue to
write to it. It uncovered a bug with an unhandled nil pointer. It
also uncovered some issues in the reconnection logic that were fixed
at the same time.