api/v1 test sometimes makes parser panic

[roidelapluie]

What did you do?

Run go test 5 times in github.com/prometheus/prometheus/web/api/v1

What did you expect to see?

ok five times

What did you see instead? Under which circumstances?

ok five times but 2 times with a panic

Environment

• System information:

  Linux

• Prometheus version:

  master

• Logs:

$ go test ; go test ; go test ; go test ; go test
parser panic: interface conversion: interface {} is *parser.seriesDescription, not *parser.VectorSelector
goroutine 400 [running]:
github.com/prometheus/prometheus/promql/parser.(*parser).recover(0xc0000f3000, 0xc000bc9b78)
        /home/roidelapluie/go/src/github.com/prometheus/prometheus/promql/parser/parse.go:273 +0x122
panic(0x1655280, 0xc001466ab0)
        /home/roidelapluie/godist/go/src/runtime/panic.go:967 +0x166
github.com/prometheus/prometheus/promql/parser.ParseMetricSelector(0xc0010236cd, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/roidelapluie/go/src/github.com/prometheus/prometheus/promql/parser/parse.go:155 +0x1df
github.com/prometheus/prometheus/web/api/v1.(*API).deleteSeries(0xc000256c30, 0xc00025bb00, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/roidelapluie/go/src/github.com/prometheus/prometheus/web/api/v1/api.go:1309 +0x6a1
github.com/prometheus/prometheus/web/api/v1.TestAdminEndpoints.func4(0xc0001a6ea0)
        /home/roidelapluie/go/src/github.com/prometheus/prometheus/web/api/v1/api_test.go:2091 +0x2fd
testing.tRunner(0xc0001a6ea0, 0xc00142b720)
        /home/roidelapluie/godist/go/src/testing/testing.go:992 +0xdc
created by testing.(*T).Run
        /home/roidelapluie/godist/go/src/testing/testing.go:1043 +0x357
PASS
ok      github.com/prometheus/prometheus/web/api/v1     0.072s
PASS
ok      github.com/prometheus/prometheus/web/api/v1     0.072s
PASS
ok      github.com/prometheus/prometheus/web/api/v1     0.069s
PASS
ok      github.com/prometheus/prometheus/web/api/v1     0.068s
parser panic: interface conversion: interface {} is *parser.seriesDescription, not *parser.VectorSelector
goroutine 393 [running]:
github.com/prometheus/prometheus/promql/parser.(*parser).recover(0xc00049a000, 0xc000421b78)
        /home/roidelapluie/go/src/github.com/prometheus/prometheus/promql/parser/parse.go:273 +0x122
panic(0x1655280, 0xc00156ee70)
        /home/roidelapluie/godist/go/src/runtime/panic.go:967 +0x166
github.com/prometheus/prometheus/promql/parser.ParseMetricSelector(0xc001520cad, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/roidelapluie/go/src/github.com/prometheus/prometheus/promql/parser/parse.go:155 +0x1df
github.com/prometheus/prometheus/web/api/v1.(*API).deleteSeries(0xc0000b6c30, 0xc000d7f200, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/roidelapluie/go/src/github.com/prometheus/prometheus/web/api/v1/api.go:1309 +0x6a1
github.com/prometheus/prometheus/web/api/v1.TestAdminEndpoints.func4(0xc00055a6c0)
        /home/roidelapluie/go/src/github.com/prometheus/prometheus/web/api/v1/api_test.go:2091 +0x2fd
testing.tRunner(0xc00055a6c0, 0xc0000a0d20)
        /home/roidelapluie/godist/go/src/testing/testing.go:992 +0xdc
created by testing.(*T).Run
        /home/roidelapluie/godist/go/src/testing/testing.go:1043 +0x357
PASS
ok      github.com/prometheus/prometheus/web/api/v1     0.069s

[roidelapluie]

cc @slrtbtfs

The input that creates the panic is "123".

[roidelapluie]

That is probably a race between the recover() and the end of the test?

[slrtbtfs]

New hypothesis:

It's because the parser struct is being reused to save allocations.

At

prometheus/promql/parser/parse.go

Lines 166 to 179 in 5ab6b04

	func newParser(input string) *parser {
	p := parserPool.Get().(*parser)
	p.injecting = false
	p.parseErrors = nil
	// Clear lexer struct before reusing.
	p.lex = Lexer{
	input: input,
	state: lexStatements,
	}
	return p
	}

the generatedParserResult field is not reset to nil and the old data lying around can mess up the parser error handling.

It should only cause problems in case the parser fails, though.

This likely is the cause of both this issue and #7127.

[roidelapluie]

Indeed, this patch fixes it:

From 5afa6bc2069fb43234954514453d2d46c35b1008 Mon Sep 17 00:00:00 2001
From: Julien Pivotto <roidelapluie@inuits.eu>
Date: Thu, 16 Apr 2020 01:44:43 +0200
Subject: [PATCH] Cleanup generatedParserResult

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
---
 promql/parser/parse.go | 1 +
 1 file changed, 1 insertion(+)

diff --git a/promql/parser/parse.go b/promql/parser/parse.go
index 1cdfd75da..3d5ebed4b 100644
--- a/promql/parser/parse.go
+++ b/promql/parser/parse.go
@@ -168,6 +168,7 @@ func newParser(input string) *parser {
 
        p.injecting = false
        p.parseErrors = nil
+       p.generatedParserResult = nil
 
        // Clear lexer struct before reusing.
        p.lex = Lexer{
-- 
2.22.0

[roidelapluie]

closed by #7132