search

nautilus / gateway

A federated api gateway for graphql services. https://gateway.nautilus.dev/
MIT License
396 stars 50 forks source link

Fix data race when building insertion points #132

Closed prokaktus closed 3 years ago

prokaktus commented 3 years ago

Data race happens when executeStep iterates over many Then steps.

Here that happens: https://github.com/nautilus/gateway/blob/c001b083e6a63c18bbc23f5ada6fb14c81285085/execute.go#L262

insertionPoint re-used for many steps and new iteration of loop could affect previous, because oldBranch not copied in executorFindInsertionPoints. I think it happens somewhere there: https://github.com/nautilus/gateway/blob/c001b083e6a63c18bbc23f5ada6fb14c81285085/execute.go#L453

-race flag catch this with warning on specific query:

WARNING: DATA RACE
Read at 0x00c00099a1f0 by goroutine 79:
  github.com/nautilus/gateway.executeStep()
      /Users/prokaktus/prg/repos/gateway/execute.go:175 +0x708

Previous write at 0x00c00099a1f0 by goroutine 67:
  github.com/nautilus/gateway.executorFindInsertionPoints()
      /Users/prokaktus/prg/repos/gateway/execute.go:463 +0x2144
  github.com/nautilus/gateway.executeStep()
      /Users/prokaktus/prg/repos/gateway/execute.go:265 +0x1ae0

Goroutine 79 (running) created at:
  github.com/nautilus/gateway.executeStep()
      /Users/prokaktus/prg/repos/gateway/execute.go:282 +0x2224

Goroutine 67 (finished) created at:
  github.com/nautilus/gateway.executeStep()
      /Users/prokaktus/prg/repos/gateway/execute.go:282 +0x2224

I create this PR as a Draft, because I want to try add test with example for that.

AlecAivazis commented 3 years ago

@prokaktus - is this ready to go? What do you think about adding the check for race conditions in the github workflows?

prokaktus commented 3 years ago

@AlecAivazis I think this is really good idea! I could help with that in separate PR. May I write in Slack #nautilus channel to discuss implementation details?

I'll try to add test today or tomorrow.

prokaktus commented 3 years ago

@AlecAivazis I've added test case. You could reproduce original problem, if you run test with -race flag without fix, e.g.:

go test -race -run TestExecutor_plansWithManyDeepDependencies
==================
WARNING: DATA RACE
Read at 0x00c00019e0b0 by goroutine 11:
  github.com/nautilus/gateway.executeStep()
      /Users/prokaktus/prg/repos/gateway/execute.go:175 +0x4e8

Previous write at 0x00c00019e0b0 by goroutine 10:
  github.com/nautilus/gateway.executorFindInsertionPoints()
      /Users/prokaktus/prg/repos/gateway/execute.go:456 +0xf34
  github.com/nautilus/gateway.executeStep()
      /Users/prokaktus/prg/repos/gateway/execute.go:262 +0xf30

Goroutine 11 (running) created at:
  github.com/nautilus/gateway.executeStep()
      /Users/prokaktus/prg/repos/gateway/execute.go:275 +0x110c

Goroutine 10 (running) created at:
  github.com/nautilus/gateway.executeStep()
      /Users/prokaktus/prg/repos/gateway/execute.go:275 +0x110c
==================
--- FAIL: TestExecutor_plansWithManyDeepDependencies (0.00s)
    execute_test.go:2253: Encountered error executing plan: Could not find id in path
    testing.go:1093: race detected during execution of test
FAIL
exit status 1
FAIL    github.com/nautilus/gateway 3.174s
AlecAivazis commented 3 years ago

Thanks so much for adding the tests! And yes, you can absolutely reach out to me on slack in that channel

AlecAivazis commented 3 years ago

This has now been released in version v0.1.15