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

Closer errors are ignored. #614

Closed
gregwebs opened this issue Jun 28, 2019 · 5 comments · Fixed by #653
Closed

Closer errors are ignored. #614

gregwebs opened this issue Jun 28, 2019 · 5 comments · Fixed by #653
Assignees

Comments

@gregwebs
Copy link
Contributor

gregwebs commented Jun 28, 2019

Bug Report

In this commit the errors from DeferClose were all ignored. Its possible that we might decide we want to ignore all Closer errors (one option would be just to log them), but then we should have a different function that doesn't pass in an error param.

Note that I currently have PRs #581 #618 open that moves this file (so changes to this file will have merge conflicts with it).

@gregwebs gregwebs changed the title Closer errors ignored. Closer errors are ignored. Jun 28, 2019
@xiaojingchen
Copy link
Contributor

xiaojingchen commented Jul 4, 2019

@gregwebs you mean add a new function as follow

func DeferClose(c io.Closer) {
	if err := c.Close(); err != nil {
		// log err
	}
}

is it right?

@gregwebs
Copy link
Contributor Author

gregwebs commented Jul 4, 2019

Yes.

@xiaojingchen
Copy link
Contributor

At present, there is no need for this method. I will add it when needs it.

@gregwebs
Copy link
Contributor Author

gregwebs commented Jul 9, 2019

The code doesn't make sense right now since it passes in a pointer to an error and then doesn't use the error. I would suggest putting a link to this issue at the current DeferClose function so that the code can be understood.

@xiaojingchen
Copy link
Contributor

I would like to remove the error pointer, directly to print error log when failed to close. the closing error should not expose to outside.

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 a pull request may close this issue.

3 participants