schema.VisitJSONString contains DATA RACE

[alexanderbolgov-ef]

Hello!
Thanks for creating and supporting this library

I've noticed data race, let me show.

Consider following sample test:

import (
	"github.com/getkin/kin-openapi/openapi3"
	"testing"
)

func TestRace_test(t *testing.T) {
	var schema = openapi3.Schema{
		Pattern:                     "race",
	}
	go func() { _ = schema.VisitJSONString("test")}()
	_ = schema.VisitJSONString("test")
}

when I run this test, I get the following output:

=== RUN   TestRace_test
==================
WARNING: DATA RACE
Write at 0x00c00011f658 by goroutine 7:
  github.com/getkin/kin-openapi/openapi3.(*Schema).visitJSONString()
      /Users/alexanderbolgov/go/pkg/mod/github.com/getkin/[email protected]/openapi3/schema.go:1144 +0xf84
  github.com/getkin/kin-openapi/openapi3.(*Schema).VisitJSONString()
      /Users/alexanderbolgov/go/pkg/mod/github.com/getkin/[email protected]/openapi3/schema.go:1086 +0x70
  ef-studio/tf/shr.TestRace_test()
      /Users/alexanderbolgov/ef-code/ef-studio/backend/go/tf/shr/test-race_test.go:13 +0xd2
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1194 +0x202

Previous read at 0x00c00011f658 by goroutine 8:
  github.com/getkin/kin-openapi/openapi3.(*Schema).visitJSONString()
      /Users/alexanderbolgov/go/pkg/mod/github.com/getkin/[email protected]/openapi3/schema.go:1142 +0x14a6
  github.com/getkin/kin-openapi/openapi3.(*Schema).VisitJSONString()
      /Users/alexanderbolgov/go/pkg/mod/github.com/getkin/[email protected]/openapi3/schema.go:1086 +0x70
  ef-studio/tf/shr.TestRace_test.func1()
      /Users/alexanderbolgov/ef-code/ef-studio/backend/go/tf/shr/test-race_test.go:12 +0x4d

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1239 +0x5d7
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1512 +0xa6
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1194 +0x202
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1510 +0x612
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1418 +0x3b3
  main.main()
      _testmain.go:43 +0x236

Goroutine 8 (running) created at:
  ef-studio/tf/shr.TestRace_test()
      /Users/alexanderbolgov/ef-code/ef-studio/backend/go/tf/shr/test-race_test.go:12 +0xaf
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1194 +0x202
==================
    testing.go:1093: race detected during execution of test
--- FAIL: TestRace_test (0.00s)

=== CONT  
    testing.go:1093: race detected during execution of test
FAIL

Looks like the problem sits here:

I believe this can happen when a web server receives two concurrent requests and performs two concurrent validations

[fenollp]

Note this lib isn't claiming (yet) to be thread-safe.

You are correct this is where you are experiencing a data race: this change inhibits the race.

package openapi3_test

import (
	"github.com/getkin/kin-openapi/openapi3"
	"testing"
)

func TestRace_test(t *testing.T) {
	var schema = openapi3.Schema{
		Pattern: "race",
	}
	_ = schema.VisitJSONString("test")
	go func() { _ = schema.VisitJSONString("test") }()
	_ = schema.VisitJSONString("test")
}

(This simply ensures compiledPattern!=nil before concurrent access).

In the future a Compile method will be provided, in part for #230. This would be the place to do this kind of thing. In the meantime I suggest using this trick unless you have a better idea.

[alexanderbolgov-ef]

thanks for the answer
Could you suggest a way to compile all patterns for the whole openapi3.T object?
Then I could call this method on the app startup to prevent possible races

[fenollp]

Visit all schemas looking for nonempty Pattern and call VisitJSONString on each with whatever data, disregarding the potential error. Then all compiledPattern would be set.

[alexanderbolgov-ef]

Hello again.
Would it make sense improve Schema.validate() method and compile a schema.Pattern if schema.Type == 'string' && schema.Format == 'regex' at the moment of validation?
somewhere around here github.com/getkin/[email protected]/openapi3/schema.go:668

If yes, I will be happy to submit a PR

[fenollp]

Yes that makes sense :)

[fenollp]

Also please make sure to add a test that runs with -race in CI.

[alexanderbolgov-ef]

Hey @fenollp
I've created a PR including your test that reproduces a race condition. Thanks for doing that
#375