Concerns about testing.T.Context in Go 1.8

[Joe Tsai]

Coming in Go 1.8, the proposal, issue #16221, adds a method on testing.T that returns a Context that is canceled when the Test function returns.

This feature is really useful when writing tests with multiple goroutines as a way to signal to those goroutines that it is time to terminate. However, in using the Context method, I feel like it is still lacking. In addition to signaling of termination, I want the testing framework to also wait until all goroutines have finished before starting the next test to ensure that they do not have any side effects on the next test.

As a possible approach, we could add some method that returns a *sync.WaitGroup. When the Test function returns, the testing framework implicitly cancels the Context and then calls WaitGroup.Wait(). 

The downside to this is that we would now be adding two additional methods (Context and WaitGroup) to testing.T, which already has a fairly large API surface. Also, most tests are probably not running with multiple goroutines.

On the flipside, this would assists concurrent tests from needing to manually create a Context and a WaitGroup and then defer on canceling and waiting on them. It also cleans up helper functions since they would only need to take in a testing.T object as opposed to (testing.T, context.Context, sync.WaitGroup).

My concluding thoughts; we should either add both:

• A mechanism to signal termination of goroutines (achieved via the Context method)
  
• A mechanism for the testing framework to wait until all goroutines to finish (achieved via a WaitGroup method)

Or

• We should not add anything at all.

What are people's thoughts?

JT

[ne...@misago.org]

I agree.

A test's goroutines should not outlive the test function. (This is a special case of the more general rule that a function should not be outlived by the goroutines it starts, unless it provides some way for the caller to wait on those goroutines exiting.) Canceling a test's context after the test function has returned is too late.

We could add some additional mechanism to wait on a test's goroutines, such as a (*testing.T).WaitGroup or (*testing.T).Go method. However, testing.T already has a large surface and these methods only save four lines of boilerplate. Most tests do not involve concurrency. I don't think it's unreasonable to expect the ones that do to begin with:

  ctx, cancel := context.WithCancel(context.Background)

  defer cancel()

  var wg sync.WaitGroup

  defer wg.Wait()

That said, a per-test Context might provide an interesting mechanism to plumb a timeout into tests.

[Brad Fitzpatrick]

I don't think there's anything to change in the testing package.

As Neil noted, users who wish to have their goroutines be done afterwards can arrange for that, either deferring a WaitGroup.Wait, or something like net/http's "func afterTest".

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Damien Neil]

On Fri, Dec 2, 2016 at 2:01 PM Brad Fitzpatrick <brad...@golang.org> wrote:

I don't think there's anything to change in the testing package.

As Neil noted, users who wish to have their goroutines be done afterwards can arrange for that, either deferring a WaitGroup.Wait, or something like net/http's "func afterTest".

That doesn't work with the proposed (*testing.T).Context, however. The Context is canceled after the test returns, so tests have no way to insert a post-cancellation wait. Any test which uses (*testing.T).Context will leak goroutines by design.

[Brad Fitzpatrick]

Do you include goroutines shutting down in under a millisecond after the test failure a "leak"? I think that's acceptable for almost all tests.

If a test really wants to wait for goroutines on its way out, it can defer a cancel of a separate signal+wait, as it can today with this API change.

This mechanism wasn't made to guarantee the property that by the time a test func ended, zero goroutines would be outstanding.

[Damien Neil]

(oops, wrong reply button)

On Fri, Dec 2, 2016 at 2:36 PM, Brad Fitzpatrick <brad...@golang.org> wrote:

Do you include goroutines shutting down in under a millisecond after the test failure a "leak"? I think that's acceptable for almost all tests.

Yes, I consider that a leak and don't think it is acceptable in a well-written test.

"Under a millisecond" is optimistic under tsan builds and when accounting for goroutines which do significant cleanup work. Goroutines that outlive the test cannot call t.Log or t.Error, they may produce logs which will be interleaved with those of other tests, and they may interfere with subsequent tests in other ways.

Tests should clean up after themselves. That includes waiting for goroutines to exit.

[Joe Tsai]

Another thing to consider is that any goroutine that has access to t.Context also has access to t.Errorf.

You can imagine how this combined with Context can cause problems:

select {

...

case <-t.Context().Done():

    if err := something.Close(); err != nil {

        t.Errorf("unexpected Close error: %v", err)

    }

}

This is problematic since it is violation of the testing API to call t.Errorf after the Test function has returned.

JT

--
You received this message because you are subscribed to a topic in the Google Groups "golang-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-dev/rS6psxIf17Q/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-dev+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Joe Tsai
http://www.digital-static.net/

[Gustavo Niemeyer]

Hi Brad,

On Fri, Dec 2, 2016 at 8:36 PM, Brad Fitzpatrick <brad...@golang.org> wrote:

Do you include goroutines shutting down in under a millisecond after the test failure a "leak"? I think that's acceptable for almost all tests.

Such a flagging mechanism that by definition will only be useful when things will continue running in the background past the test completion does feel like a wart to me. We should be discouraging people from doing that in the first place, because arbitrary background logic delayed for arbitrary periods of time while other things are being tested is a common source of testing pain, breaking in curious and hard to reproduce ways.

As people pointed out, it's not a mere case of putting logic locally. The context, by definition, will only trigger code to *start* aborting after it's too late. An aborting yet running goroutine can do whatever it wants in order to attempt to stop cleanly.

In my own testing, services generally have a Stop method because of that. Internally, they leverage a tomb [1] to flag the stopping and block until everything is done cleaning themselves up. Stop also usually returns an error, so one can also check that the stop was itself clean.

[1] http://gopkg.in/tomb.v2

gustavo @ http://niemeyer.net

[Steven Hartland]

This reminds me that tests are run in parallel anyway, which has bitten us in the past, but it also means that if you think things are running serially you'll likely be disappointed.

    Regards
    Steve

[Gustavo Niemeyer]

No, not within a package. Consider the existence of https://golang.org/pkg/testing/#T.Parallel.

--

You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

gustavo @ http://niemeyer.net