Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Datarace at server shutdown #145

Closed
alexvanin opened this issue Apr 18, 2022 · 3 comments · Fixed by #149
Closed

Datarace at server shutdown #145

alexvanin opened this issue Apr 18, 2022 · 3 comments · Fixed by #149
Assignees
@alexvanin
Milestone
v0.20.0

Comments

@alexvanin
Copy link
Contributor

alexvanin commented Apr 18, 2022 •
edited

Related to #130 and #131

Sometimes I still get data race errors in integration test. These errors are inconsistent, see https://github.com/nspcc-dev/neofs-http-gw/runs/6061957808 (tests are passed, but data coverage failed due to data race error).

The issue is that we use RequestCtx in NeoFS API calls. Client from neofs-sdk-go creates new context with cancellation, and it triggers RequestCtx.Done(). This method is not thread-safe, because returned channel is set to nil at server shutdown. So data race detector sometimes can be triggered on this.

I don't think this fasthttp behaviour will be changed. But providing separate context to the NeoFS API calls seems like an overhead for me to solve corner-case at the shutdown. So maybe the better option is to drop data race check in integration test once again.

WARNING: DATA RACE
Write at 0x00c0001aef80 by goroutine 69:
  github.com/valyala/fasthttp.(*Server).Shutdown()
      /home/runner/go/pkg/mod/github.com/valyala/fasthttp@v1.34.0/server.go:1906 +0x2b6
  github.com/nspcc-dev/neofs-http-gw.(*app).Serve.func1()
      /home/runner/work/neofs-http-gw/neofs-http-gw/app.go:207 +0x93

Previous read at 0x00c0001aef80 by goroutine 60:
  github.com/valyala/fasthttp.(*RequestCtx).Done()
      /home/runner/go/pkg/mod/github.com/valyala/fasthttp@v1.34.0/server.go:2704 +0x56
  context.propagateCancel.func1()
      /opt/hostedtoolcache/go/1.17.8/x64/src/context/context.go:280 +0x66

Goroutine 69 (running) created at:
  github.com/nspcc-dev/neofs-http-gw.(*app).Serve()
      /home/runner/work/neofs-http-gw/neofs-http-gw/app.go:205 +0x138
  github.com/nspcc-dev/neofs-http-gw.runServer·dwrap·2()
      /home/runner/work/neofs-http-gw/neofs-http-gw/integration_test.go:70 +0x67

Goroutine 60 (finished) created at:
  context.propagateCancel()
      /opt/hostedtoolcache/go/1.17.8/x64/src/context/context.go:278 +0x264
  context.WithCancel()
      /opt/hostedtoolcache/go/1.17.8/x64/src/context/context.go:237 +0x10e
  github.com/nspcc-dev/neofs-sdk-go/client.(*Client).ObjectSearchInit()
      /home/runner/go/pkg/mod/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.3.0.20220407103316-e50e6d28280d/client/object_search.go:282 +0x671
  github.com/nspcc-dev/neofs-sdk-go/pool.(*Pool).SearchObjects.func1()
      /home/runner/go/pkg/mod/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.3.0.20220407103316-e50e6d28280d/pool/pool.go:1320 +0x10b
  github.com/nspcc-dev/neofs-sdk-go/pool.(*Pool).call()
      /home/runner/go/pkg/mod/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.3.0.20220407103316-e50e6d28280d/pool/pool.go:905 +0x105
  github.com/nspcc-dev/neofs-sdk-go/pool.(*Pool).SearchObjects()
      /home/runner/go/pkg/mod/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.3.0.20220407103316-e50e6d28280d/pool/pool.go:1317 +0x709
  github.com/nspcc-dev/neofs-http-gw/downloader.(*Downloader).search()
      /home/runner/work/neofs-http-gw/neofs-http-gw/downloader/download.go:355 +0x5a6
  github.com/nspcc-dev/neofs-http-gw/downloader.(*Downloader).byAttribute()
      /home/runner/work/neofs-http-gw/neofs-http-gw/downloader/download.go:314 +0x96c
  github.com/nspcc-dev/neofs-http-gw/downloader.(*Downloader).DownloadByAttribute()
      /home/runner/work/neofs-http-gw/neofs-http-gw/downloader/download.go:295 +0x4a
  github.com/nspcc-dev/neofs-http-gw/downloader.(*Downloader).DownloadByAttribute-fm()
      /home/runner/work/neofs-http-gw/neofs-http-gw/downloader/download.go:294 +0x22
  github.com/nspcc-dev/neofs-http-gw.(*app).logger.func1()
      /home/runner/work/neofs-http-gw/neofs-http-gw/app.go:267 +0x70b
  github.com/fasthttp/router.(*Router).Handler()
      /home/runner/go/pkg/mod/github.com/fasthttp/router@v1.4.1/router.go:414 +0xa90
  github.com/fasthttp/router.(*Router).Handler-fm()
      /home/runner/go/pkg/mod/github.com/fasthttp/router@v1.4.1/router.go:402 +0x44
  github.com/valyala/fasthttp.(*Server).serveConn()
      /home/runner/go/pkg/mod/github.com/valyala/fasthttp@v1.34.0/server.go:2341 +0x1a44
  github.com/valyala/fasthttp.(*Server).serveConn-fm()
      /home/runner/go/pkg/mod/github.com/valyala/fasthttp@v1.34.0/server.go:2084 +0x4d
  github.com/valyala/fasthttp.(*workerPool).workerFunc()
      /home/runner/go/pkg/mod/github.com/valyala/fasthttp@v1.34.0/workerpool.go:224 +0xeb
  github.com/valyala/fasthttp.(*workerPool).getCh.func1()
      /home/runner/go/pkg/mod/github.com/valyala/fasthttp@v1.34.0/workerpool.go:196 +0x53
@alexvanin alexvanin mentioned this issue Apr 18, 2022
Merged
@KirillovDenis
Copy link
Contributor

But providing separate context to the NeoFS API calls seems like an overhead for me to solve corner-case at the shutdown

Are we sure that there is only one corner-case?

@alexvanin
Copy link
Contributor Author

But providing separate context to the NeoFS API calls seems like an overhead for me to solve corner-case at the shutdown

Are we sure that there is only one corner-case?

Concurrent read/write access to the Done() channel of RequestCtx is only possible at shutdown, because done channel variable is not modified in runtime.

@alexvanin
Copy link
Contributor Author

Check if requestCtx is closed when client terminates connection.

masterSplinter01 pushed a commit that referenced this issue Apr 22, 2022
@alexvanin @masterSplinter01
[#145] Use application context in NeoFS API requests
1e3df95 
It is meaningless to use RequestCtx as a context.Context
for NeoFS operation, because context won't be closed
until application shutdown. Moreover, it also triggers
data race detection, because server's done channel, which
is accessible for reading from RequestCtx, is set to `nil`.

Using application context doesn't change gateway behavior,
but it suppresses data race trigger at shutdown. It also
allows possibility to set configurable timeouts for NeoFS
networking if we will ever need them.

Signed-off-by: Alex Vanin <alexey@nspcc.ru>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@alexvanin alexvanin

Labels
None yet
Projects
None yet
Milestone
v0.20.0
Development

Successfully merging a pull request may close this issue.

Use application context in NeoFS API requests
2 participants
@alexvanin @KirillovDenis