hclwrite: fix data race in formatSpaces()

[ryancragun]

This fixes a data race in formatSpaces() by inlining shared state into the caller.

Before

==================
WARNING: DATA RACE
Write at 0x0000017bf900 by goroutine 14:
  github.com/hashicorp/hcl/v2/hclwrite.formatSpaces()
      /Users/ryan/code/hashi/hcl/hclwrite/format.go:127 +0x29d
  github.com/hashicorp/hcl/v2/hclwrite.format()
      /Users/ryan/code/hashi/hcl/hclwrite/format.go:36 +0x6c
  github.com/hashicorp/hcl/v2/hclwrite.TokensForValue()
      /Users/ryan/code/hashi/hcl/hclwrite/generate.go:25 +0x8a
  github.com/hashicorp/hcl/v2/hclwrite.NewExpressionLiteral()
      /Users/ryan/code/hashi/hcl/hclwrite/ast_expression.go:61 +0x97
  github.com/hashicorp/hcl/v2/hclwrite.(*Body).SetAttributeValue()
      /Users/ryan/code/hashi/hcl/hclwrite/ast_body.go:167 +0xd4
  github.com/hashicorp/hcl/v2/hclwrite.TestRoundTripSafeConcurrent.func1()
      /Users/ryan/code/hashi/hcl/hclwrite/round_trip_test.go:179 +0xee

Previous write at 0x0000017bf900 by goroutine 35:
  github.com/hashicorp/hcl/v2/hclwrite.formatSpaces()
      /Users/ryan/code/hashi/hcl/hclwrite/format.go:127 +0x29d
  github.com/hashicorp/hcl/v2/hclwrite.format()
      /Users/ryan/code/hashi/hcl/hclwrite/format.go:36 +0x6c
  github.com/hashicorp/hcl/v2/hclwrite.TokensForValue()
      /Users/ryan/code/hashi/hcl/hclwrite/generate.go:25 +0x8a
  github.com/hashicorp/hcl/v2/hclwrite.NewExpressionLiteral()
      /Users/ryan/code/hashi/hcl/hclwrite/ast_expression.go:61 +0x97
  github.com/hashicorp/hcl/v2/hclwrite.(*Body).SetAttributeValue()
      /Users/ryan/code/hashi/hcl/hclwrite/ast_body.go:167 +0xd4
  github.com/hashicorp/hcl/v2/hclwrite.TestRoundTripSafeConcurrent.func1()
      /Users/ryan/code/hashi/hcl/hclwrite/round_trip_test.go:179 +0xee

Goroutine 14 (running) created at:
  github.com/hashicorp/hcl/v2/hclwrite.TestRoundTripSafeConcurrent()
      /Users/ryan/code/hashi/hcl/hclwrite/round_trip_test.go:176 +0x34
  testing.tRunner()
      /usr/local/Cellar/go/1.17.6/libexec/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/Cellar/go/1.17.6/libexec/src/testing/testing.go:1306 +0x47

Goroutine 35 (finished) created at:
  github.com/hashicorp/hcl/v2/hclwrite.TestRoundTripSafeConcurrent()
      /Users/ryan/code/hashi/hcl/hclwrite/round_trip_test.go:176 +0x34
  testing.tRunner()
      /usr/local/Cellar/go/1.17.6/libexec/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/Cellar/go/1.17.6/libexec/src/testing/testing.go:1306 +0x47
==================
FAIL
FAIL    github.com/hashicorp/hcl/v2/hclwrite    0.690s
?       github.com/hashicorp/hcl/v2/hclwrite/fuzz/config        [no test files]
FAIL

My apologies for the messy diff, my editor runs gofumpt and yamllint so that's where the noise is coming from. If it's a problem let me know and I'll update it.

Signed-off-by: Ryan Cragun me@ryan.ec

[apparentlymart]

Hi @ryancragun! Thanks for tracking these down.

For nilToken I can see clearly where the issue is: it's trying to share the nilToken object between multiple other objects but then they are presumably racing to try to update SpacesBefore during formatting, which assumes that each token is distinct. I believe the shared nilToken was an early thing which I neglected to address when later implementing the formatSpaces function, which for the first time introduced the idea of mutating tokens in-place.

I feel less sure about the inKeyword part. My expectation is that the object would be initialized once at startup and then never modified again, because we're using it only to match against another token. Is there something more tricky going on there which I can't see just from this diff?

[ryancragun]

@apparentlymart I believe you're right on point. While I was moving nilToken I also inlined inKeyword as it was only used once and I thought perhaps it was a good time to remove it. I'm happy to put inKeyword back if you prefer that.

[apparentlymart]

Hi @ryancragun,

I don't think it makes a huge amount of difference in practice, so I think what you did here is fine too... I was just focused on trying to understand what the data races were here so I could think about the consequences of these changes. 😀

Thanks for checking!

[apparentlymart]

I also checked on that failing Windows test and it seems like for some reason the Go toolchain thinks it needs to compile some C code in order to build HCL now.

I don't see anything in your changeset here that introduces any new dependencies that might require CGo, so I guess probably what's happening is that the Go toolchain needs to rebuild everything in order to get race-detector-enabled versions of the standard library packages. If that is the reason then I think that should be fine, but I guess it means we'll need to make gcc available in whatever Windows image that win-test-go-1.12 step is running in.

Since I believe we're intending to move away from CircleCI to GitHub Actions in the not-too-distant future anyway, perhaps a reasonable compromise for now would be to enable the race detector only for the linux tests for now, and then once we're on GitHub Actions we can use a more exhaustive build matrix and rely on the fact that the GitHub Actions images ship with a more reasonable set of base packages for software testing.

[ryancragun]

I have precisely this PR's worth of experience using circleci, so that was my best guess at how to achieve only running the race detector on linux. 😄

[apparentlymart]

I don't have very sophisticated knowledge of CircleCI myself (and I'm intentionally not learning more because we're going to move away from it before too long anyway!), but what you did here seems reasonable to me, especially with the expectation that this won't be in use very long before it gets replaced, and so it working is the most important thing.

[ryancragun]

@apparentlymart small PSA that I dropped the circleci changes and rebased this.

[hashicorp-cla]

All committers have signed the CLA.

[alisdair]

Thanks for this!