go_test: Allow not registering a SIGTERM handler

[illicitonion]

What type of PR is this?

Bug fix Feature

What does this PR do? Why is it needed?

The rules_go SIGTERM handler causes tests which themselves test responding to SIGTERM to panic and fail.

[linzhp]

This helps, but we simply add signal.Reset(syscall.SIGTERM) to the test cases or TestMain of them that register SIGTERM handler. @illicitonion did you try that approach?

[illicitonion]

This helps, but we simply add signal.Reset(syscall.SIGTERM) to the test cases or TestMain of them that register SIGTERM handler. @illicitonion did you try that approach?

Ooh that's a nice idea! Yeah, this approach works, and it seems reasonable (though a little noisy) for tests which are explicitly handling signals themselves, but it feels much noisier to have to add to all tests if they use goleak...

[fmeum]

goleak will come with a default ignorelist entry in the future as far as I understood @linzhp.

I do prefer the Go-only solution in that case if we document it.

[linzhp]

Good point on goleak. Uber maintains a internal patch of all known leaks, but there is no plan to put the list in the open source goleak project. It's unfair to ask every user of goleak to maintain a patch. So in combination of the SIGTERM reset and goleak opt-out, this PR is quite useful, unless we have better implementation of the timeout handler.

[fmeum]

The more I think about it, the less I think this new attribute provides a good user experience: It's used to work around two separate issues, both of which would better be fixed in other layers.

For the few tests that require bespoke SIGTERM handling, the solution of manually resetting the handler from Go seems much more targeted and discoverable. It also communicates the intent better and guards against other conflicting handlers other libraries may register in init.

For goleak, disabling the new timeout handler just to avoid the false positive is also not great as it means users have to choose between using goleak and having better timeout reporting.

@linzhp Would goleak be open to contributions to improve this interaction?

[linzhp]

I am not a maintainer of goleak, based on this reply, they don't want a default ignore list, given that goleak supports IgnoreAnyFunction.

So tests that uses goleak can call VerifyNone(t, goleak.IgnoreAnyFunction("github.com/bazelbuild/rules_go/go/tools/bzltestutil.RegisterTimeoutHandler.func1"))) or

func TestMain(m *testing.M) {
  goleak.VerifyTestMain(m, goleak.IgnoreAnyFunction("github.com/bazelbuild/rules_go/go/tools/bzltestutil.RegisterTimeoutHandler.func1"))
}

[fmeum]

I followed up on the goleak thread, maybe we can solve this by introducing an ignore mechanism that rules_go can use and goleak doesn't have to maintain.