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

Merged
merged 6 commits into from
Aug 14, 2020

Conversation

kachayev
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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

@ztellman
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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?

@kachayev kachayev changed the base branch from master to 1.0.0 August 14, 2020 00:14
@kachayev kachayev added the 1.0 label Aug 14, 2020
@kachayev kachayev merged commit bfd977d into clj-commons:1.0.0 Aug 14, 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.

An attempt to send non-existing file as a server response hangs up the connection
2 participants