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

server/conn: remove EOF log #432

Merged
merged 3 commits into from
Dec 19, 2016
Merged

server/conn: remove EOF log #432

merged 3 commits into from
Dec 19, 2016

Conversation

huachaohuang
Copy link
Contributor

@huachaohuang huachaohuang commented Dec 11, 2016

EOF means the remote peer closes the connection actively, seems not a server error, we don't need to log it as error to confuse users.

@siddontang
Copy link
Contributor

If the pd leader steps down, it will also close all connections, what's the error here? do we need to ignore it too?

@siddontang
Copy link
Contributor

Any update?

@huachaohuang
Copy link
Contributor Author

If a connection is closed, the error will be "use of closed network connection".
However, that error is not exported (net.errClosing), but we can match the string to detect it.
PTAL @siddontang @overvenus

if errors.Cause(err) == io.EOF {
return false
}
if strings.Contains(err.Error(), "use of closed network connection") {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can check the err is net.Error instead.
It's better not to check the error message directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we only want to ignore this specified error, not all of the net.Error.

Copy link
Contributor

Choose a reason for hiding this comment

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

define a const var for the error message.

add test to cover this function.

@siddontang
Copy link
Contributor

LGTM

@huachaohuang
Copy link
Contributor Author

PTAL @overvenus

Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

lgtm

@huachaohuang huachaohuang merged commit 712779f into master Dec 19, 2016
@huachaohuang huachaohuang deleted the huachao/remove-eof branch December 19, 2016 05:01
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.

3 participants