tigrawap / slit

slit - a modern PAGER for viewing logs, get more than most in less time
Other
469 stars 18 forks source link

prevent race conditions #64

Closed olshevskiy87 closed 6 years ago

olshevskiy87 commented 6 years ago

hi! the command go run -race cmd/slit/main.go -- ... with the large file as an argument outputs the following data race warnings:

==================
WARNING: DATA RACE
Read at 0x00c4200bc0d8 by goroutine 74:
  github.com/tigrawap/slit.(*Fetcher).filteredLine()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:70 +0xea
  github.com/tigrawap/slit.(*Fetcher).lineBuilder.func1.2()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:192 +0x7d

Previous write at 0x00c4200bc0d8 by main goroutine:
  github.com/tigrawap/slit.(*Slit).Display()
      /home/dv/godir/src/github.com/tigrawap/slit/slit.go:99 +0xaa
  main.main()
      /home/dv/godir/src/github.com/tigrawap/slit/cmd/slit/main.go:96 +0x66e

Goroutine 74 (running) created at:
  github.com/tigrawap/slit.(*Fetcher).lineBuilder.func1()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:191 +0x455
==================
Found 1 data race(s)
exit status 66

==================
WARNING: DATA RACE
Read at 0x00c4200bc0d8 by goroutine 90:
  github.com/tigrawap/slit.(*Fetcher).filteredLine()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:70 +0xea
  github.com/tigrawap/slit.(*Fetcher).lineBuilder.func1.2()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:192 +0x7d

Previous write at 0x00c4200bc0d8 by main goroutine:
  github.com/tigrawap/slit.(*Slit).Display()
      /home/dv/godir/src/github.com/tigrawap/slit/slit.go:99 +0xaa
  main.main()
      /home/dv/godir/src/github.com/tigrawap/slit/cmd/slit/main.go:96 +0x66e

Goroutine 90 (running) created at:
  github.com/tigrawap/slit.(*Fetcher).lineBuilder.func1()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:191 +0x455
==================
==================
WARNING: DATA RACE
Read at 0x00c4200bc0c0 by goroutine 12:
  github.com/tigrawap/slit.(*Fetcher).readline()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:133 +0x5c
  github.com/tigrawap/slit.(*Fetcher).Get.func1()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:226 +0xbd

Previous write at 0x00c4200bc0c0 by main goroutine:
  github.com/tigrawap/slit.(*Fetcher).seek()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:127 +0x1fb
  github.com/tigrawap/slit.(*Slit).Display()
      /home/dv/godir/src/github.com/tigrawap/slit/slit.go:100 +0x11f
  main.main()
      /home/dv/godir/src/github.com/tigrawap/slit/cmd/slit/main.go:96 +0x66e

Goroutine 12 (running) created at:
  github.com/tigrawap/slit.(*Fetcher).Get()
      /home/dv/godir/src/github.com/tigrawap/slit/fetcher.go:222 +0x1f1
  github.com/tigrawap/slit.(*Slit).CanFitDisplay()
      /home/dv/godir/src/github.com/tigrawap/slit/slit.go:264 +0x138
  main.tryDirectOutputIfShort()
      /home/dv/godir/src/github.com/tigrawap/slit/cmd/slit/main.go:102 +0xb0
  main.main()
      /home/dv/godir/src/github.com/tigrawap/slit/cmd/slit/main.go:81 +0x7ae

Here is my attempt to prevent such behavior, PTAL. Maybe it's awkward or the wrong way, so I'm waiting for your comments.

tigrawap commented 6 years ago

Thanks for paying attention, but I try to avoid mutexes in hot paths

Problem here was leak of fetcher from input length validator

Fixed in ae59b9ccfd43bb3670348ed160f6ffe7fcd62fee Please confirm that it fixed in your scenario as well

olshevskiy87 commented 6 years ago

@tigrawap , unfortunately the last commit (https://github.com/tigrawap/slit/commit/ae59b9ccfd43bb3670348ed160f6ffe7fcd62fee) didn't solve the issue. I've just run the same command I wrote before and receive the same warnings.

$ git rev-parse HEAD
ae59b9ccfd43bb3670348ed160f6ffe7fcd62fee

$ go version
go version go1.10.3 linux/amd64

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="off"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/dv/godir"
GORACE=""
GOROOT="/usr/lib/go-1.10"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.10/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build736289794=/tmp/go-build -gno-record-gcc-switches"
tigrawap commented 6 years ago

You are right, did not fix.

Followed up with f984387ede44425896d9186222312592865adaa7

olshevskiy87 commented 6 years ago

now everything is fine)

olshevskiy87 commented 6 years ago

actually I found another race condition. when I resize my terminal while running go run -race ... the following warnings are appear:

WARNING: DATA RACE
Write at 0x00c42026c010 by main goroutine:
  github.com/tigrawap/slit.(*viewer).resize()
      /home/dv/godir/src/github.com/tigrawap/slit/term.go:408 +0x65
  github.com/tigrawap/slit.(*viewer).termGui()
      /home/dv/godir/src/github.com/tigrawap/slit/term.go:477 +0xb6b
  github.com/tigrawap/slit.(*Slit).Display()
      /home/dv/godir/src/github.com/tigrawap/slit/slit.go:109 +0x2af
  main.main()
      /home/dv/godir/src/github.com/tigrawap/slit/cmd/slit/main.go:96 +0x66e

Previous read at 0x00c42026c010 by goroutine 126:
  github.com/tigrawap/slit.(*viewer).refreshIfEmpty()
      /home/dv/godir/src/github.com/tigrawap/slit/term.go:567 +0x205
  github.com/tigrawap/slit.(*viewer).termGui.func2()
      /home/dv/godir/src/github.com/tigrawap/slit/term.go:461 +0x4c

Goroutine 126 (finished) created at:
  github.com/tigrawap/slit.(*viewer).termGui()
      /home/dv/godir/src/github.com/tigrawap/slit/term.go:461 +0x4c5
  github.com/tigrawap/slit.(*Slit).Display()
      /home/dv/godir/src/github.com/tigrawap/slit/slit.go:109 +0x2af
  main.main()
      /home/dv/godir/src/github.com/tigrawap/slit/cmd/slit/main.go:96 +0x66e
tigrawap commented 6 years ago

Fixed in 3ede8a075b9bf413671ede10a1ea2af392f72ccb

Not a fan myself, not obvious, might happen later in another place.
But again, right now I'd like not to pass every call to every place via lock, since main routine is single threaded and better to avoid mutexes in hot paths (these are not)