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

proposal: runtime/race: add const Enabled #36477

Closed
ainar-g opened this issue Jan 9, 2020 · 31 comments
Closed

proposal: runtime/race: add const Enabled #36477

ainar-g opened this issue Jan 9, 2020 · 31 comments

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Jan 9, 2020

I've seen a lot of code like this:

// +build race

package p

const race = true
// +build !race

package p

const race = false

I think it would be better to have one boolean for that, and the most logical place for it seems to be either runtime/debug or runtime/race.

@ericlagergren
Copy link
Contributor

I’ve done this in the past to skip tests that were only useful with the -race flag set.

@ianlancetaylor ianlancetaylor changed the title runtime/debug: add const Race proposal: runtime/debug: add const Race Jan 9, 2020
@gopherbot gopherbot added this to the Proposal milestone Jan 9, 2020
@josharian
Copy link
Contributor

It seems to me a more natural place for this would be runtime/race.Enabled.

@josharian
Copy link
Contributor

Note that the standard library has this already in internal/race.

@ainar-g
Copy link
Contributor Author

ainar-g commented Jan 9, 2020

@josharian, I can send a CL with something like:

package race

import "internal/race"

// Enabled tells whether the race detector was enabled when building.
const Enabled = enabled

const enabled = race.Enabled

If the proposal is accepted.

@mvdan
Copy link
Member

mvdan commented Mar 5, 2020

I too, have used this method manually in the past. The main counterargument here could be that one shouldn't make code behave differently when running in race mode - but that's already possible today, and I think there are plenty of legitimate use cases such as:

  • skipping tests that should only run in race mode
  • skipping tests that can't run in race mode, e.g. if they try to do a go build on a platform that doesn't support it yet
  • to propagate -race, for example if a test executes go run

@mvdan
Copy link
Member

mvdan commented Mar 5, 2020

I also think that, long term, we could replace internal/race.Enabled with runtime/race.Enabled throughout the repository. @ainar-g I assume you want to retitle the proposal, given that it still mentions runtime/debug.Race.

@mvdan
Copy link
Member

mvdan commented Mar 5, 2020

Here's another valid use case, I think: #37681

If the proposal is accepted, runtime/race.Enabled could be used by third party libraries to implement similar "leaked without being closed" finalizer checks.

@ainar-g ainar-g changed the title proposal: runtime/debug: add const Race proposal: runtime/race: add const Enabled Mar 5, 2020
@bcmills
Copy link
Member

bcmills commented Mar 11, 2020

The “tests that should only run in race mode” are precisely (and only) those tests that verify run-time checks that are implemented using the race detector itself and do not build a new binary using go build or similar. (Any other check can be tested using a boolean, build tag, or environment variable.) Is that set significant at the moment? If not, perhaps a build tag suffices.

Propagating the -race flag to go run seems like a similarly niche use-case.

@bcmills
Copy link
Member

bcmills commented Mar 11, 2020

The set of “tests that can't run in race mode” should be empty. A variable whose value is set at compile-time is too late for guarding go build (since you could reasonably build the test itself in non-race mode even on a platform for which the -race tag is fully supported). For the go build use-case, we would need a separate predicate, more like the existing cmd/internal/sys.RaceDetectorSupported.

@bcmills
Copy link
Member

bcmills commented Mar 11, 2020

So, I agree that this would be useful in some cases, but those cases seem pretty rare — I'm not sure that they're worth the moral hazard of using the boolean to paper over actual races.

@josharian
Copy link
Contributor

Another use case: In the compiler, when race mode is enabled, we randomize the order in which we compile functions. This is because the optimal ordering for performance was the least likely to flush out race conditions.

navytux added a commit to navytux/go123 that referenced this issue Apr 5, 2020
Provide race.Enabled for programs to know whether they were built with
race detector or not.

Code originates from https://lab.nexedi.com/kirr/wendelin.core/tree/25c3184d/wcfs/internal/race
See also: golang/go#36477.
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 6, 2021
@josharian
Copy link
Contributor

Another use case: Reducing the running time of a test in race mode. I encountered a test recently that experienced a drastic slowdown when run with -race, because of the heavy use of synchronization. Reducing the iterations with -race was the only reasonable fix I saw.

@bcmills
Copy link
Member

bcmills commented Feb 22, 2021

@josharian, maybe? But you can also reduce the running time of a test using testing.Short, and that's more-or-less orthogonal to the race detector.

(Plus, if one of your tests fails due to memory corruption in an ordinary run, it seems unpleasant for that test to change its behavior — perhaps dramatically! — when you run it under the race detector to try to diagnose the problem.)

@josharian
Copy link
Contributor

But you can also reduce the running time of a test using testing.Short, and that's more-or-less orthogonal to the race detector.

You can, but as you point out, testing.Short is orthogonal. It seems better to tie the solution to the actual problem; I'd rather go test -race complete in a reasonable length of time.

it seems unpleasant for that test to change its behavior

It doesn't. It just runs less long; it's a stress test that runs for fewer iterations. If I were investigating memory corruption, I'd run it in the loop. Or read the test and notice the line that says if race.Enabled { iterations /= 10 }.

@DeedleFake
Copy link

Every argument in favor of this seems to be primarily for tests, not for normal code. It seems that everyone agrees that normal code should basically never do something different depending on the presence of the race detector, and that the very rare exceptions to that can be handled just fine by the build tags, as they provide enough annoyance in so implementing something that it at least discourages you from doing so.

I personally have written a manual Enabled boolean for the race detector using the build tags and I also only used it for tests. In my case, Like @josharian's case, I had tests that iterated a number of times, and the race detector degraded the performance to the point that it took a prohibitively long time to run with the race detector, as in it was timing out the CI system, but the detection of races was more than sufficient after just a few iterations, whereas the general logic test was better to run a large number of times.

I'm personally somewhat ambivalent on the presence of a boolean on top of the build tags, but, all of that being said, why not put it in something more test specific, like testing/race, or even just inside testing itself?

@josharian
Copy link
Contributor

@DeedleFake sometimes you test without package testing. And sometimes the place you want to add the hook is not in a test file. See my comment: #36477 (comment).

@josharian
Copy link
Contributor

Another use for this flag: When the race detector is enabled, profiling could emit warnings.

(Where and how to emit those warnings will vary; package testing might print them to stderr, third party packages might return an error from some API, etc. In an ideal world, we'd be able to write a message to the pprof output itself, which cmd/pprof would then display.)

@anatol
Copy link

anatol commented Oct 13, 2021

It would be great to have a flag that specifies whether race detector is enabled or not.

Plenty of projects at github use the "build flag" approach https://github.com/search?p=1&q=%22Package+israce+reports+if+the+Go+race+detector+is+enabled.%22&type=Code

And in my case I need to propagate "-race" to "go build" command I invoke from tests. See anatol/booster@35b1dda

But it would be convenient to have something cleaner (like runtime.race.Enabled variable).

@josharian
Copy link
Contributor

Another use case (observed long ago, finally remembered to record here): Skipping AllocsPerRun tests in race mode, because -race can cause extra allocs.

@ainar-g
Copy link
Contributor Author

ainar-g commented Oct 19, 2021

Another use case that I seem to not have brought up sooner are debug outputs, like --verbose --version or metrics. The race detector is one of the checks one might enable at build stage, and having a single, standard way to tell if the binary was actually built with it would be useful.

@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

It seems like we have a variety of use cases, including some abuse cases. All the cases are already handled, with some complexity, by using build tags. Adding race.Enabled would make all the cases easier. The question is whether we want to encourage people to change test behavior based on the race detector. Today it's clear that's not something you're supposed to do, because it's so difficult to do it. Adding race.Enabled would make it seem like something you're expected to do, at least some of the time.

@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@ainar-g
Copy link
Contributor Author

ainar-g commented Aug 17, 2022

Another non-test-related use case I've encountered since my last comment is tweaking resource usage. Since programs compiled with --race generally require more RAM and CPU time, some defaults might need to be changed, such as numbers of workers or cache sizes. I don't recall, if the race detector still has the 8192-goroutine limit, but this could be another reason.

@rsc
Copy link
Contributor

rsc commented Aug 31, 2022

As I noted two weeks ago, the lack of race.Enabled today implies that most people shouldn't be using it in most contexts.
That's probably the right implication, and something we should keep, which would mean we should decline this issue.
(People can still work around it if they are sure they need it, but that extra work is useful to discourage such efforts.)

Do I have that right?

@aclements
Copy link
Member

I don't recall, if the race detector still has the 8192-goroutine limit, but this could be another reason.

This limitation at least has been lifted on Linux and MacOS (and we're working on lifting it on Windows).

@ainar-g
Copy link
Contributor Author

ainar-g commented Sep 1, 2022

@rsc, I do not agree that it's currently “difficult”. It is merely annoying. If your goal is to discourage any difference between race and non-race code, you could remove the race build tags as well, and that would really make it difficult. And I think that neither of these approaches are right.

@mvdan, @josharian, and I provided valid examples of non-test use cases, including propagation of the --race flag value to go calls, changing the number of resources used in race mode due to increased resource usage and other limitations, metrics output, debug checks, and some others. Some of the test-related examples in this thread are valid as well. @anatol has provided proof that the annoying approach is currently being used.

Leaving only the annoying approach would be approximately the same as not having GOOS and GOARCH and forcing people to only use build tags and const isWindows = true/false instead, at least to me.

I have no strong feelings either way, but the decision against this proposal will merely mean that this boilerplate code will propagate by copy-paste, because nobody would want to add an external dependency for a single boolean. (At least in the Go realm, heh.)

@ainar-g
Copy link
Contributor Author

ainar-g commented Sep 5, 2022

Another use case is working around #37233, which seems to have been around for a while now. This fits into the “Work Around Race Detector's Issues” category of use cases, along with the goroutine limit (soon to be gone) and RAM/CPU usage.

@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

@ainar-g thanks for the reminder about #37233. That seems like a bug that we should fix rather than a reason for race.Enabled.

@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@mvdan
Copy link
Member

mvdan commented Sep 8, 2022

I still mildly support this proposal because I've had to resort to //go:build race a few too many times over the years. But I also admit that I don't represent the average Go programmer, so it's hard for me to judge whether the advantage of the convenience would outweigh the potential disadvantage of misuse or overuse.

@rsc
Copy link
Contributor

rsc commented Sep 21, 2022

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

10 participants