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

Fix #1824: HEAD request on static files causes app to stop #1825

Merged
merged 3 commits into from
Aug 23, 2017

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Aug 22, 2017

The problem was that for HEAD requests specifically, we implement
an explicit Content-Length header (normally we let httpuv figure
out the Content-Length based on the content, but for HEAD we don't
return any content but still want to include the Content-Length).

The Content-Length header was only implemented correctly for string
values, not for raw vectors or file-by-path. This change implements
the value correctly for all currently valid httpuv content.

The problem was that for HEAD requests specifically, we implement
an explicit Content-Length header (normally we let httpuv figure
out the Content-Length based on the content, but for HEAD we don't
return any content but still want to include the Content-Length).

The Content-Length header was only implemented correctly for string
values, not for raw vectors or file-by-path. This change implements
the value correctly for all currently valid httpuv content.
@jcheng5
Copy link
Member Author

jcheng5 commented Aug 22, 2017

@wch, I didn't include code to only unlink if in a temp dir. It's clear from the unlink docs that if recursive=FALSE it won't try to remove a directory, not even an empty one; I thought that was good enough. Let me know if you disagree.

NULL
}

if (is.na(result)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When would the result be NA? The cases I can think of are an NA string input, and when file.info() is called on a file that doesn't exist. A comment about this would be helpful so we remember.

R/middleware.R Outdated
getResponseContentLength <- function(response, deleteOwnedContent) {
force(deleteOwnedContent)

result <- if (is.character(response$content)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like you're trying to be strict about checking the input, and so it might be good to check that the length is 1.

@wch
Copy link
Collaborator

wch commented Aug 23, 2017

Other than a couple small comments, it looks good to me.

@wch wch merged commit c2b3c33 into master Aug 23, 2017
@wch wch deleted the joe/bugfix/head-crash branch August 23, 2017 18:01
@wch wch removed the review label Aug 23, 2017
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.

2 participants