code review 4810060: exp/template: fix endless loop

exp/template: fix endless loop

No progress was made in indirect() if the reflect.Value
was an non-nil and non-empty interface.

[gri, 2011-07-30 02:16:26]

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

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

[r, 2011-07-30 03:35:35]

LGTM

it does look like a bug

can you add a test that broke before but is fixed now?

http://codereview.appspot.com/4810060/diff/4001/src/pkg/exp/template/exec.go
File src/pkg/exp/template/exec.go (right):

http://codereview.appspot.com/4810060/diff/4001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:590: return v, true
break

http://codereview.appspot.com/4810060/diff/4001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:596: return v, false
return v, v.IsNil()

[gri, 2011-07-30 16:48:40]

1) Your suggestions won't work, see below.

2) Do you have a concrete suggestion for a test? This is an endless loop; and at
the moment I can only think of testing this w/ goroutines and a time-out
channel, but w/o I/O I don't know that the scheduler will get to the time-out
goroutine. Happy to try but it might be a bit of code for what looks like an
obvious bug, in retrospect. Please advise.

http://codereview.appspot.com/4810060/diff/4001/src/pkg/exp/template/exec.go
File src/pkg/exp/template/exec.go (right):

http://codereview.appspot.com/4810060/diff/4001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:590: return v, true
On 2011/07/30 03:35:35, r wrote:
> break

That won't work, see below.

http://codereview.appspot.com/4810060/diff/4001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:596: return v, false
On 2011/07/30 03:35:35, r wrote:
> return v, v.IsNil()

The for loop may never be entered in which case v.IsNil() may be called with
values that cannot be nil - and then v.IsNil() will panic.

[r2, 2011-07-30 16:52:13]

On Jul 30, 2011, at 9:48 AM, gri@golang.org wrote:

> 1) Your suggestions won't work, see below.

you're right.

> 2) Do you have a concrete suggestion for a test? This is an endless
> loop; and at the moment I can only think of testing this w/ goroutines
> and a time-out channel, but w/o I/O I don't know that the scheduler will
> get to the time-out goroutine. Happy to try but it might be a bit of
> code for what looks like an obvious bug, in retrospect. Please advise.

as i said, i was looking for a test that is fixed by this change, so you don't
need to do anything with timeouts, just see for yourself that it loops, put in
your fix, and see it complete.

-rob

[gri, 2011-07-30 17:00:48]

>> 2) Do you have a concrete suggestion for a test? This is an endless
>> loop; and at the moment I can only think of testing this w/ goroutines
>> and a time-out channel, but w/o I/O I don't know that the scheduler will
>> get to the time-out goroutine. Happy to try but it might be a bit of
>> code for what looks like an obvious bug, in retrospect. Please advise.
>
> as i said, i was looking for a test that is fixed by this change, so you don't
need to do anything with timeouts, just see for yourself that it loops, put in
your fix, and see it complete.

The godoc rewrite using the new templates doesn't hang anymore with
this fix; while it hung before. That is, there is an implicit test via
godoc, once its new version is submitted.

Will submit.
- Robert

[r2, 2011-07-30 17:01:50]

On Jul 30, 2011, at 10:00 AM, Robert Griesemer wrote:

>>> 2) Do you have a concrete suggestion for a test? This is an endless
>>> loop; and at the moment I can only think of testing this w/ goroutines
>>> and a time-out channel, but w/o I/O I don't know that the scheduler will
>>> get to the time-out goroutine. Happy to try but it might be a bit of
>>> code for what looks like an obvious bug, in retrospect. Please advise.
>> 
>> as i said, i was looking for a test that is fixed by this change, so you
don't need to do anything with timeouts, just see for yourself that it loops,
put in your fix, and see it complete.
> 
> The godoc rewrite using the new templates doesn't hang anymore with
> this fix; while it hung before. That is, there is an implicit test via
> godoc, once its new version is submitted.

sure, submit, but i was hoping for a little test case to prevent regression. 

-rob

[gri, 2011-07-30 19:24:36]

PTAL.

Added a test case that terminates now, and didn't w/o the fix.

[r, 2011-07-30 21:24:59]

LGTM
thanks

[gri, 2011-07-31 00:12:00]

*** Submitted as http://code.google.com/p/go/source/detail?r=aff321288dc0 ***

exp/template: fix endless loop

No progress was made in indirect() if the reflect.Value
was an non-nil and non-empty interface.

R=r, r
CC=golang-dev
http://codereview.appspot.com/4810060