Expressions with functions are not goroutine-safe #43

a13xb · 2020-01-01T03:08:00Z

I'm using this library to batch parse a large number of HTML documents in parallel (via antchfx/htmlquery) and it will always result either in a nil pointer dereference or wrong result.

The issue is reuse of query objects due to shallow cloning of functionQuery objects.

htmlquery calls xpath.Expr.Select, which creates a new node iterator containing a cloned query.
The query is cloned recursively.
Eventually functionQuery is cloned but the clone is shallow, because the function is actually a closure that captured references to the original query objects when it was constructed.
At this point the all cloned queries will reference the same original child query nodes that will be mutated concurrently when Select is called (for example attributeQuery.iterator in attributeQuery.Select).

Here is a self-contained example with htmlquery, which uses a global cache of xpath.Expr objects, that hits the problem fairly quickly on my machine.

package main

import (
	"github.com/antchfx/htmlquery"
	"golang.org/x/net/html"
	"log"
	"strings"
)

func work() {
	doc, err := html.Parse(strings.NewReader(`
		<html>
			<head><title>Hello</title></head>
			<body>
				<div class="c1 c2">A</div>
				<div class="c2 c3">B</div>
				<div class="c3 c1">C</div>
			</body>
		</html>
	`))
	if err != nil {
		log.Fatal(err)
	}
	ns := htmlquery.Find(doc, `//div[contains(@class,"c1")]/text()`)
	if len(ns) == 0 {
		log.Fatal("text not found")
	}
	elems := []string{}
	for _, n := range ns {
		elems = append(elems, htmlquery.InnerText(n))
	}
	log.Printf(`%d elems: %v`, len(elems), elems)
}

func main() {
	for i := 0; i < 4; i++ {
		go func() {
			for {
				work()
			}
		}()
	}
	select {}
}

Note that it will run correctly if you remove the contains clause, and just use =, for example.

My current workaround is to disable query cache in htmlquery, but what really needs to be done is to refactor functionQuery so it can be cloned correctly.

The text was updated successfully, but these errors were encountered:

allows clone about `functionQuery` input arguments which implemented `query` interface.

zhengchun · 2020-01-03T06:55:05Z

Thanks for your feedback. I added a new method to decide whether clone input argument. This might help.

zhengchun · 2020-01-10T10:43:13Z

Closed, fixed in v1.1.4.

zhengchun added bug enhancement labels Jan 1, 2020

zhengchun added a commit that referenced this issue Jan 3, 2020

#43 makes xpath function are goroutine-safe.

c523c29

allows clone about `functionQuery` input arguments which implemented `query` interface.

zhengchun closed this as completed Jan 10, 2020

d-g-laptev mentioned this issue Jun 13, 2020

avoid nil pointer panic (thread save) #51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expressions with functions are not goroutine-safe #43

Expressions with functions are not goroutine-safe #43

a13xb commented Jan 1, 2020 •

edited

Loading

zhengchun commented Jan 3, 2020

zhengchun commented Jan 10, 2020

Expressions with functions are not goroutine-safe #43

Expressions with functions are not goroutine-safe #43

Comments

a13xb commented Jan 1, 2020 • edited Loading

zhengchun commented Jan 3, 2020

zhengchun commented Jan 10, 2020

a13xb commented Jan 1, 2020 •

edited

Loading