code review 5901045: net/http/fs.go: Changed the placement of call to checkL...

net/http/fs.go: Changed the placement of call to checkLastModified. It is being called too early, causing it to check the directory itself rather than the index file, which it should if there is one. This causes a return of "304 Not Modified" status and stale index pages on the client side.

[haavid, 2012-03-24 23:06:26]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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

[bradfitz, 2012-03-25 00:55:09]

Thanks!

Without looking into this yet, this is believably valid, and looks
low-risk.  We're very close to cutting Go 1, though, so we're taking few
changes.  If you could include a test (in fs_test.go) that fails without
this change and passes with it, I'd be much more willing to take this on
short notice.


On Sat, Mar 24, 2012 at 4:06 PM, <haavid@falch.net> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http/fs.go: Changed the placement of call to checkLastModified. It
> is being called too early, causing it to check the directory itself
> rather than the index file, which it should if there is one. This causes
> a return of "304 Not Modified" status and stale index pages on the
> client side.
>
> Please review this at
http://codereview.appspot.com/**5901045/<http://codereview.appspot.com/5901045/>
>
> Affected files:
>  M src/pkg/net/http/fs.go
>
>
> Index: src/pkg/net/http/fs.go
> ==============================**==============================**=======
> old mode 100644
> new mode 100755
> --- a/src/pkg/net/http/fs.go
> +++ b/src/pkg/net/http/fs.go
> @@ -243,9 +243,6 @@
>
>        // use contents of index.html for directory, if present
>        if d.IsDir() {
> -               if checkLastModified(w, r, d.ModTime()) {
> -                       return
> -               }
>                index := name + indexPage
>                ff, err := fs.Open(index)
>                if err == nil {
> @@ -260,6 +257,9 @@
>        }
>
>        if d.IsDir() {
> +               if checkLastModified(w, r, d.ModTime()) {
> +                       return
> +               }
>                dirList(w, f)
>                return
>        }
>
>
>

[rsc, 2012-03-26 13:14:36]

Let's leave this for after Go 1.
At this point I'd rather ship with minor bugs
we know about than the bugs we introduce
by editing code.  Thanks.

Russ

[haavid, 2012-03-28 00:16:25]

There is certainly is a big day coming up, and this isn't a serious error. I'll
update the patch and submit a test along with it, after Go 1 has launched. Added
it to the issue tracker in the meantime.

On 2012/03/26 13:14:36, rsc wrote:
> Let's leave this for after Go 1.
> At this point I'd rather ship with minor bugs
> we know about than the bugs we introduce
> by editing code.  Thanks.
> 
> Russ

[rsc, 2012-06-03 04:48:39]

Are you interested in writing a test for this and getting it in, now that Go 1
is out?

[haavid, 2012-06-09 16:56:50]

On 2012/06/03 04:48:39, rsc wrote:
> Are you interested in writing a test for this and getting it in, now that Go 1
> is out?

I'm sorry, but I don't have the time for it these days.

I did look into it earlier, and didn't find any truly elegant way to test for
this bug. Effective ways sure, but a bit messy.
The nature of the bug calls for a file generated on the fly during the test,
then updated with a newer timestamp to check if the server picks up on it.
Fix is trivial, the only tests I came up with were messy.

If my schedule clears up soon I would look at it again, but it might be better
if someone else can handle this.

[bradfitz, 2012-06-11 15:45:24]

On Sat, Jun 9, 2012 at 9:56 AM, <haavid@falch.net> wrote:

> On 2012/06/03 04:48:39, rsc wrote:
>
>> Are you interested in writing a test for this and getting it in, now
>>
> that Go 1
>
>> is out?
>>
>
> I'm sorry, but I don't have the time for it these days.
>
> I did look into it earlier, and didn't find any truly elegant way to
> test for this bug. Effective ways sure, but a bit messy.
> The nature of the bug calls for a file generated on the fly during the
> test, then updated with a newer timestamp to check if the server picks
> up on it.
> Fix is trivial, the only tests I came up with were messy.
>
> If my schedule clears up soon I would look at it again, but it might be
> better if someone else can handle this.


I wrote a test for it here: http://codereview.appspot.com/6300081

Thanks for finding the problem!

- Brad

[bradfitz, 2012-07-30 00:53:20]

Ignore all this.  I was trying to use the keyboard to scroll the page, but
JavaScript onselectionchanged in the drop-down was running instead.

This CL is old.


On Sun, Jul 29, 2012 at 5:50 PM, <bradfitz@golang.org> wrote:

> R=nigeltao
>
> (sent by gocodereview)
>