Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Skip rendering of template by the autopage handler on HTTP methods other than GET #1140

Closed
wants to merge 1 commit into from

Conversation

racke
Copy link
Member

@racke racke commented Mar 11, 2016

Tiny optimization for autopage handler - it doesn't make sense to render the template and throw it away.

@veryrusty
Copy link
Member

rfc 1945 - HTTP 1.0 section 8.2 and (the obsolete) rfc 2616 - HTTP 1.1 state

The metainformation contained in the HTTP headers in response to a HEAD request should be identical to the information sent in response to a GET request

i.e. A HEAD request should include a Content-Length header.

Whereas rfc 7231 - HTTP 1.1 section 4.3.2 allows Content-Length header to be missing.

After #1139 was merged, the remaining logic in the AutoPage handler this Pr is optimising does need some cleaning up. However, I think we should always render the content and let our middleware stack remove the body for a HEAD request, so the Content-Length header is included in the response.

@veryrusty
Copy link
Member

and .. @racke++ for picking up that the logic needs cleaning up there!

@xsawyerx
Copy link
Member

I think we should benchmark not using the Head middleware. If we're already at the spot where we have the body and headers and we can just remove the body, that will save at least two subroutine calls per request for a single if() condition. It's much better.

(Some of the optimizations I did in D2 to make it faster than D1 was removing unnecessary middlewares - it made a big difference.)

@xsawyerx
Copy link
Member

xsawyerx commented May 26, 2016

So, so summarize:

  • I agree with @veryrusty on the spec. HEAD should be GET minus the content, meaning we do need to run the GET, just not return the content.
  • I will open a different ticket to remove the HEAD middleware. Middlewares are more expensive than we assume.

Should we close this issue?

@xsawyerx
Copy link
Member

Done - #1170.

@cromedome
Copy link
Contributor

Doing some housekeeping... looking at this PR, the ensuing discussion, and #1170, is it ok to close this? I am inclined to. Thanks!

@SysPete
Copy link
Member

SysPete commented Mar 29, 2020

@cromedome agree: time to close this one.

@cromedome
Copy link
Contributor

Thanks!

@cromedome cromedome closed this Mar 30, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants