-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Read websocket close in tail handler #1383
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.
Thanks for the PR! Changes look good to me.
I think we should also change logcli to watch for SIGINT
and SIGTERM
signals to gracefully terminate websocket connection. Do you want to do that change?
Can you create an issue @sandlis and then we can merge this ? |
This comment has been minimized.
This comment has been minimized.
89d3d7d
to
c8030f3
Compare
I've done the change to gracefully terminate the logcli websocket on |
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.
Thanks for the new changes! Just a small change suggested, otherwise it LGTM.
<-stopChan | ||
if err := conn.WriteMessage(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseNormalClosure, "")); err != nil { | ||
log.Println("Error closing websocket:", 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.
Let us add os.Exit(0)
after this line since older builds won't handle this close message which makes cli program to continue running since we are handling those interrupts and waiting for the server to gracefully close the connection to break the loop down below.
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.
Good point about the older server builds. Since we are exiting here no need to introduce a change to the main loop anymore.
c8030f3
to
555e93e
Compare
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.
LGTM
Added support for graceful closing of the tail query endpoint when the websocket client sends a close message. This fixes the minor issue of errors always being logged when the client disconnects i.e.
"Error writing ping message to websocket"
and"Error writing close message to websocket"
.