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

testing: call (*M).after on signal or internal-generated panic #19394

Closed
codesenberg opened this issue Mar 4, 2017 · 12 comments

Comments

Projects
None yet
7 participants
@codesenberg
Copy link
Contributor

commented Mar 4, 2017

What version of Go are you using (go version)?

go version go1.8 windows/amd64

What operating system and processor architecture are you using (go env)?

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\frei\Documents\Go\Workspaces\Default
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\frei\AppData\Local\Temp\go-build564910375=/tmp/go-build -gno-record-gcc-switches
set CXX=g++
set CGO_ENABLED=1
set PKG_CONFIG=pkg-config
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2

What did you do?

Ran go test -cpuprofile cpu.pprof -memprofile mem.pprof -timeout 10s in a folder with the following test file:

package main

import (
	"flag"
	"runtime"
	"testing"
	"time"
)

var (
	testDuration = flag.Duration(
		"duration", 30*time.Second, "how long to run tests",
	)
	workers = flag.Uint("workers", 10, "number of workers")
)

func TestDummy(t *testing.T) {
	for i := uint(0); i < *workers; i++ {
		go func() {
			counter := 0
			for {
				counter++
				runtime.Gosched()
			}
		}()
	}
	time.Sleep(*testDuration)
}

What did you expect to see?

Tests to end with a panic and non-empty profiles in cpu.pprof and mem.pprof.

What did you see instead?

Tests ended with a panic, but cpu.pprof was empty and mem.pprof wasn't created at all.

Previous discussion on #golang-nuts: link.

/cc @ianlancetaylor

@codesenberg codesenberg changed the title testing: -cpuprofile and -memprofile produce empty profiles or no profile at all, when tests end with a panic, because of a timeout testing: -cpuprofile and -memprofile produce empty profile or no profile at all, when tests end with a panic, because of a timeout Mar 4, 2017

@josharian josharian added this to the Go1.9 milestone Mar 4, 2017

@ALTree

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

As I see it, running with a timeout set means "this shouldn't take more than 10 seconds, abort the program if it does". It's not meant to be used as a knob to control benchmarking duration; and you shouldn't use it this way. Your program was aborted because you started it with a flag set that means: "this should never take more than 10 seconds, please panic if it does".

So the question here is: should we guarantee that partial profiles are written to file if the program is killed/aborted because an invalid condition / unrecoverable problem occurred?

I don't think we should.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

The reason I would like to write partial profiles if the program is killed (at least internally) is that sometimes I start a large benchmarking run (e.g. math/big with -count=20) and forget to bump up the timeout, and the test gets killed, and I lose 10 minutes worth of profiling efforts.

@ALTree

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

@josharian to solve that problem maybe we could ignore the default timeout value (10m) when go test detects we're running benchmarks. Or make -count N multiply the timeout by N.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

I've been bitten by this before. Having -count N multiply the timeout by N sounds like a good fix.

@codesenberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2017

@ALTree, nah, this program is just for the ease of reproduction. I discovered this issue in a context unrelated to benchmarking and thought that I should report it. How you resolve it is up to you guys. I, personally, would like to see that "partial" profile, because of the time wasted. Also, maybe, those profiles could help to resolve the issues with the code that led to this timeout. Just my 2 cents.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

Related: #12446. This just bit me again. The fix appears to be trivial: add *timeout *= len(cpuList) to the end of parseCpuList. But documenting it got hard, and I started worrying about whether anyone might really depend on -timeout being a hard timeout. Thoughts?

@ALTree

This comment has been minimized.

Copy link
Member

commented Jun 6, 2017

We're deep in the freeze with no consensus about what should be done and no CL implementing generation of partial profiles, so I'm moving this to the go1.10 milestone.

@ALTree ALTree modified the milestones: Go1.10, Go1.9 Jun 6, 2017

@ALTree ALTree added the NeedsDecision label Jun 6, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

I don't see how to fix this without adding atexit, which we've worked hard to avoid.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

I see the argument for multiplying timeout by count but on the other hand -count=100 turning into 1000m timeout might not be what you want.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

If we only care about the specific case of a signal arriving, or the internal panic generated by a timeout - that is, the intersection of the two things I just replied to - then we could arrange to call m.after() carefully ourselves before exiting. That seems OK to try at least if anyone is interested in preparing a CL.

@rsc rsc changed the title testing: -cpuprofile and -memprofile produce empty profile or no profile at all, when tests end with a panic, because of a timeout testing: call (*M).after on signal or internal-generated panic Jun 26, 2017

@rsc rsc added NeedsFix and removed NeedsDecision labels Jun 26, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Jul 14, 2017

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

@gopherbot gopherbot closed this in 7e455b6 Aug 25, 2017

@meirf

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2017

My fix was only partial but bot closed this. I used Fixes in CL, which was probably the wrong thing to do.

@golang golang locked and limited conversation to collaborators Aug 28, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.