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

Do not log Broken pipe exception #621

Closed
wants to merge 1 commit into from
Closed

Conversation

tonsky
Copy link

@tonsky tonsky commented Jun 13, 2019

When using with Immutant, Pedestal logs Broken pipe errors quite often. As far as I understand, those are nothing to be afraid of, just client silently closing TCP connection while other side is not informed about it and tries to write. This is a part of normal operation and definitely should not dominate your logs:

Screenshot 2019-06-05 at 18 26 58

@ddeaguiar
Copy link
Contributor

ddeaguiar commented Jul 5, 2019

Hi @tonsky, I really appreciate your interest in making Pedestal better! While I appreciate the intent to reduce log noise, I don't think such a specific change belongs in the error-ring-response fn. As per its doc string, this function is intended to to a catch all exception handler and it's preferable for service implementations to catch and handle exceptions in the way that best suites the service.

That being said, although possible, there is no easy way for a service implementor to handle any exception as responses are streaming. As per the http-interceptor-service-fn implementation, errors thrown by the ring-response fn are immediately handled by the error stage impl of the stylobate interceptor.

Ideally a service implementor would have the ability to provide a service-specific error handling interceptor to handle errors such as this. The bottom line is that I think a more general solution is needed. I'm going to create an issue about this and close this PR.

@ddeaguiar
Copy link
Contributor

ddeaguiar commented Jul 5, 2019

Issue #623 created.

@ddeaguiar ddeaguiar closed this Jul 5, 2019
@kommen
Copy link

kommen commented Sep 12, 2019

@ddeaguiar you probably meant to link to #623, right?

@ddeaguiar
Copy link
Contributor

@ddeaguiar you probably meant to link to #623, right?

Yep, fixed my previous comment, thanks!

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

3 participants