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

Jump to bottom

text/template/parse: Parse will leak go routines if parsing fails #10574

Closed
gravis opened this issue Apr 25, 2015 · 14 comments
Closed

text/template/parse: Parse will leak go routines if parsing fails #10574

gravis opened this issue Apr 25, 2015 · 14 comments
Labels
FrozenDueToAge
Milestone
Go1.5

Comments

@gravis
Copy link

gravis commented Apr 25, 2015

(From https://groups.google.com/forum/#!topic/golang-nuts/6nTWBLbAyKo)

When Parse() is called, a new lexer is created, and it will run in the background:

func (t *Tree) Parse(text, leftDelim, rightDelim string, treeSet map[string]*Tree, funcs ...map[string]interface{}) (tree *Tree, err error) {

[...]

t.startParse(funcs, lex(t.Name, text, leftDelim, rightDelim))

[...]

}

(https://github.com/golang/go/blob/master/src/text/template/parse/parse.go#L228)

This will start a go routine :

// lex creates a new scanner for the input string.
func lex(name, input, left, right string) *lexer {
    if left == "" {
        left = leftDelim
    }
    if right == "" {
        right = rightDelim
    }
    l := &lexer{
        name:       name,
        input:      input,
        leftDelim:  left,
        rightDelim: right,
        items:      make(chan item),
    }
    go l.run()
    return l
}

(https://github.com/golang/go/blob/master/src/text/template/parse/lex.go#L191)

Now if the parser fails to parse, it will panic, and the error is recovered :

// recover is the handler that turns panics into returns from the top level of Parse.
func (t *Tree) recover(errp *error) {
    e := recover()
    if e != nil {
        if _, ok := e.(runtime.Error); ok {
            panic(e)
        }
        if t != nil {
            t.stopParse()
        }
        *errp = e.(error)
    }
    return
}

(https://github.com/golang/go/blob/master/src/text/template/parse/parse.go#L191)

which will call t.stopParse():

// stopParse terminates parsing.
func (t *Tree) stopParse() {
    t.lex = nil
    t.vars = nil
    t.funcs = nil
}

The lexer should be GC, except it's still running in the background, because no parser is there to pull on the channel.
I think the lex should have a abort() method to end the routine cleanly, otherwise each time the parser is called and failing, we leave a running lexer behind.

I will create a pull request to fix this, as I have already some code for that.
ps: Minux has confirmed the bug http://play.golang.org/p/qTQXuT3ktV
@ianlancetaylor ianlancetaylor added this to the Go1.5Maybe milestone Apr 25, 2015
@ianlancetaylor ianlancetaylor changed the title parser.Parse will leak go routines if parsing fails go/parser: Parse will leak go routines if parsing fails Apr 25, 2015
@ianlancetaylor
Copy link
Contributor

@griesemer

@gravis
Copy link
Author

gravis commented Apr 25, 2015

Patch is ready, I'm stuck here: "Thanks, your CLA has been submitted."
I hope it won't be too long.

@bradfitz
Copy link
Contributor

Did you do a corporate CLA then I assume? Individual CLAs are instantly recognized by Gerrit.

If you did corporate, email me (@golang.org) the company name and I'll look into it.

@gravis
Copy link
Author

gravis commented Apr 25, 2015

I did a corporate first, saw the big form, and decided to go with the individual one.
Sounds like something went wrong then :(
I just had the message "Thanks, your CLA has been submitted." and nothing more.
It would be nice to unblock my account if needed.
Thanks
​

@bradfitz
Copy link
Contributor

What makes you think your account is blocked? Email me the answer. Let's not discuss in the bug tracker. Try just mailing the CL with codereview again.

@gravis
Copy link
Author

gravis commented Apr 25, 2015

Ok, nailed it.
I mixed up with my corporate account.
It's alright now! The review has been posted successfully:

https://go-review.googlesource.com/#/c/9370/

Thanks for (week-end) support guys.

@gopherbot
Copy link

CL https://golang.org/cl/9370 mentions this issue.

@mdempsky mdempsky changed the title go/parser: Parse will leak go routines if parsing fails text/template/parse: Parse will leak go routines if parsing fails Apr 26, 2015
@gravis gravis mentioned this issue Apr 29, 2015
Closed
@gravis
Copy link
Author

gravis commented Apr 29, 2015

@bradfitz I'm trying to update my changes, with a simpler version, but I can't push:

[...]
 ! [remote rejected] fix-leaking-parser -> fix-leaking-parser (prohibited by Gerrit)
error: failed to push some refs to 'https://go.googlesource.com/go'

Any idea to fix this? I can't find any thing in the guide related to this :(

Thanks,
Philippe

@bradfitz
Copy link
Contributor

Let's move Gerrit support to a mailing list.

@gravis
Copy link
Author

gravis commented Apr 29, 2015

Sorry :)

@gravis
Copy link
Author

gravis commented Apr 29, 2015

Ok, I figured out, it was indeed in the doc, under the Revise and upload section.
I had to rebase commits, but I finally manage to push a new patch.

@gopherbot
Copy link

CL https://golang.org/cl/9501 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/9658 mentions this issue.

@robpike robpike closed this as completed in 64c39a3 May 5, 2015
@gopherbot
Copy link

CL https://golang.org/cl/9841 mentions this issue.

robpike added a commit that referenced this issue May 7, 2015
@robpike
text/template: delete obsolete nil check
7bdeab1 
This was added during testing but is unnecessary.
Thanks to gravis on GitHub for catching it.

See #10574.

Change-Id: I4a8f76d237e67f5a0ea189a0f3cadddbf426778a
Reviewed-on: https://go-review.googlesource.com/9841
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@mikioh mikioh modified the milestones: Go1.5, Go1.5Maybe May 27, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
@rsc rsc unassigned robpike Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.5
Development

No branches or pull requests
6 participants
@bradfitz @gravis @mikioh @ianlancetaylor @robpike @gopherbot