FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
net/http: add If-None-Match and If-Range support to ServeContent

Please review this at http://codereview.appspot.com/6503090/

Affected files:
M src/pkg/net/http/fs.go
M src/pkg/net/http/fs_test.go

Search Discussions

  • Bradfitz at Sep 7, 2012 at 9:26 pm
    Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6503090/
  • Anonymous at Sep 7, 2012 at 9:52 pm
  • Rsc at Sep 10, 2012 at 5:02 pm
    LGTM



    http://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go
    File src/pkg/net/http/fs.go (right):

    http://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode102
    src/pkg/net/http/fs.go:102: //
    You need something more here about ETags.

    // If the caller has set w's Etag header, ServeContent uses it to handle
    requests
    // using If-Range and If-None-Match.

    http://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode127
    src/pkg/net/http/fs.go:127: if rangeReq, done = checkETag(w, r); done {
    Instead of the declarations just split the if

    rangeReq, done := checkETag(w, r)
    if done {
    return
    }

    http://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode267
    src/pkg/net/http/fs.go:267: // Invalidate the range request if the
    entity doesn't match the one
    This is getting subtle enough that it would help to have a comment
    explaining what the headers mean, both so that you can follow the code
    and so that someone else coming in with a different understanding is
    faced with the difference.

    // If-Range: version means "ignore the Range: header unless version
    matches the current file.
    // We only support ETag versions.
    // The caller must have set the ETag on the response already.
    if ir := ...

    http://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode269
    src/pkg/net/http/fs.go:269: if ir := r.Header.get("If-Range"); rangeReq
    != "" && ir != "" && ir != etag {
    Might as well drop the rangeReq != "" part. No harm, less to understand.

    http://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode277
    src/pkg/net/http/fs.go:277: // TODO(bradfitz): handle non-GET/HEAD
    requests, which
    Same thing. Also, if the GET/HEAD thing is part of If-None-Match, move
    it inside. If not, separate it with a blank line.

    // If-None-Match: version-list means "only do this request if the file
    version
    // does not match any of the versions in the list."
    if inm := r.Header.get("If-None-Match"); inm != "" {
    // Must know ETag.
    if etag == "" {
    return rangeReq, false
    }

    // Non-GET/HEAD requests require more work.
    if r.Method != "GET" && r.Method != "HEAD" {
    return rangeReq, false
    }

    // TODO(bradfitz): Deal with multiple-valued list.
    if inm == etag { ...

    Do you want || inm == "*" on that last one?

    http://codereview.appspot.com/6503090/
  • Bradfitz at Sep 10, 2012 at 5:09 pm
    https://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go
    File src/pkg/net/http/fs.go (right):

    https://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode102
    src/pkg/net/http/fs.go:102: //
    On 2012/09/10 15:55:53, rsc wrote:
    You need something more here about ETags.
    // If the caller has set w's Etag header, ServeContent uses it to handle
    requests
    // using If-Range and If-None-Match.

    Done.

    https://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode127
    src/pkg/net/http/fs.go:127: if rangeReq, done = checkETag(w, r); done {
    On 2012/09/10 15:55:53, rsc wrote:
    Instead of the declarations just split the if
    rangeReq, done := checkETag(w, r)
    if done {
    return
    }
    Done.

    https://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode267
    src/pkg/net/http/fs.go:267: // Invalidate the range request if the
    entity doesn't match the one
    On 2012/09/10 15:55:53, rsc wrote:
    This is getting subtle enough that it would help to have a comment
    explaining
    what the headers mean, both so that you can follow the code and so
    that someone
    else coming in with a different understanding is faced with the
    difference.
    // If-Range: version means "ignore the Range: header unless version
    matches the
    current file.
    // We only support ETag versions.
    // The caller must have set the ETag on the response already.
    if ir := ...
    Done.

    https://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode269
    src/pkg/net/http/fs.go:269: if ir := r.Header.get("If-Range"); rangeReq
    != "" && ir != "" && ir != etag {
    On 2012/09/10 15:55:53, rsc wrote:
    Might as well drop the rangeReq != "" part. No harm, less to
    understand.

    Done.

    https://codereview.appspot.com/6503090/diff/5001/src/pkg/net/http/fs.go#newcode277
    src/pkg/net/http/fs.go:277: // TODO(bradfitz): handle non-GET/HEAD
    requests, which
    On 2012/09/10 15:55:53, rsc wrote:
    Same thing. Also, if the GET/HEAD thing is part of If-None-Match, move it
    inside. If not, separate it with a blank line.
    // If-None-Match: version-list means "only do this request if the file version
    // does not match any of the versions in the list."
    if inm := r.Header.get("If-None-Match"); inm != "" {
    // Must know ETag.
    if etag == "" {
    return rangeReq, false
    }
    // Non-GET/HEAD requests require more work.
    if r.Method != "GET" && r.Method != "HEAD" {
    return rangeReq, false
    }
    // TODO(bradfitz): Deal with multiple-valued list.
    if inm == etag { ... Done.
    Do you want || inm == "*" on that last one?
    RFC 2616 says it's primarily for PUT races, but doesn't hurt. Done.

    "The meaning of "If-None-Match: *" is that the method MUST NOT be
    performed if the representation selected by the origin server (or by a
    cache, possibly using the Vary mechanism, see section 14.44) exists, and
    SHOULD be performed if the representation does not exist. This feature
    is intended to be useful in preventing races between PUT operations."

    https://codereview.appspot.com/6503090/
  • Bradfitz at Sep 10, 2012 at 5:43 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=67b66dd9ea4a ***

    net/http: add If-None-Match and If-Range support to ServeContent

    Also, clear Content-Type and Content-Length on Not Modified
    responses before server.go strips them and spams the logs with
    warnings.

    R=rsc
    CC=golang-dev
    http://codereview.appspot.com/6503090


    http://codereview.appspot.com/6503090/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 7, '12 at 4:19p
activeSep 10, '12 at 5:43p
posts6
users3
websitegolang.org

3 users in discussion

Bradfitz: 4 posts Rsc: 1 post Anonymous: 1 post

People

Translate

site design / logo © 2022 Grokbase