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

Catch FileNotFoundException when sending file body #471

Open
wants to merge 6 commits into
base: master
from

Conversation

@kachayev
Copy link
Collaborator

kachayev commented Jan 30, 2019

Fixes #459.

In general, we need to be more careful with the cleanup procedure and any time send-message failed for whatever uncaught reason close the connection. This specific just PR covers one of the obvious failures that the user might face. /cc #457.

@ztellman

This comment has been minimized.

Copy link
Owner

ztellman commented Mar 25, 2019

In general, I dislike the practice of the framework surfacing an exception to the client. If the application developer wants to do that, fine, but elsewhere we provide a bare 500 response and log the issue. I think I'd like to mirror that here.

@kachayev

This comment has been minimized.

Copy link
Collaborator Author

kachayev commented Mar 26, 2019

@ztellman Do you mean how error-response is implemented with stack trace printer? I just copied that from the previous implementation, but yeah... sure, that might be reworked.

@kachayev

This comment has been minimized.

Copy link
Collaborator Author

kachayev commented Mar 26, 2019

But let's finish with #485 first, these 2 have a lot of overlapping changes.

@ztellman

This comment has been minimized.

Copy link
Owner

ztellman commented Apr 19, 2019

Sorry if I misattributed where that code came from, but even if I was the one that originally did it, I think it's a mistake.

@kachayev

This comment has been minimized.

Copy link
Collaborator Author

kachayev commented Apr 19, 2019

@ztellman Oh, no problem with that. I just think that in such a case it makes sense to implement new behavior in a separated PR not to mess up the history of changes. WDYT?

kachayev added 2 commits Apr 22, 2019
…ending data and suppress stack trace printing into the response
@kachayev

This comment has been minimized.

Copy link
Collaborator Author

kachayev commented Apr 22, 2019

@ztellman Merged with the latest master changes (notably http-file functionality). I also made internal server error to look just the same for any exception that happened. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.