Skip to content
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

Merged
merged 2 commits into from
Jan 10, 2020

Conversation

beornf
Copy link
Contributor

@beornf beornf commented Dec 8, 2019

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".

@claassistantio
Copy link

claassistantio commented Dec 8, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a 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?

@cyriltovena
Copy link
Contributor

Can you create an issue @sandlis and then we can merge this ?

@beornf

This comment has been minimized.

@beornf
Copy link
Contributor Author

beornf commented Jan 9, 2020

I've done the change to gracefully terminate the logcli websocket on SIGINT and SIGTERM. How does the PR look now @sandlis?

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a 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)
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cyriltovena cyriltovena merged commit 88681a9 into grafana:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants