go-socket.io icon indicating copy to clipboard operation
go-socket.io copied to clipboard

Fix race condition.

Open googollee opened this issue 3 years ago • 6 comments

Describe the bug Race condition, see https://github.com/googollee/go-socket.io/runs/2513183135?check_suite_focus=true.

==================
WARNING: DATA RACE
Write at 0x00c00008b2a1 by goroutine 45:
  testing.(*common).FailNow()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:740 +0x4f
  testing.(*T).FailNow()
      <autogenerated>:1 +0x44
  github.com/stretchr/testify/require.Nil()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/require/require.go:955 +0x104
  github.com/stretchr/testify/require.(*Assertions).Nil()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/require/require_forward.go:748 +0xd0
  github.com/googollee/go-socket.io/engineio.TestEngineUpgrade.func1()
      /home/runner/work/go-socket.io/go-socket.io/engineio/engine_test.go:197 +0x204

Previous write at 0x00c00008b2a1 by goroutine 43:
  testing.(*common).FailNow()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:740 +0x4f
  testing.(*T).FailNow()
      <autogenerated>:1 +0x44
  github.com/stretchr/testify/require.Nil()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/require/require.go:955 +0x104
  github.com/stretchr/testify/require.(*Assertions).Nil()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/require/require_forward.go:748 +0xd0
  github.com/googollee/go-socket.io/engineio.TestEngineUpgrade()
      /home/runner/work/go-socket.io/go-socket.io/engineio/engine_test.go:282 +0x1008
  testing.tRunner()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:1193 +0x202

Goroutine 45 (running) created at:
  github.com/googollee/go-socket.io/engineio.TestEngineUpgrade()
      /home/runner/work/go-socket.io/go-socket.io/engineio/engine_test.go:186 +0x22c
  testing.tRunner()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:1193 +0x202

Goroutine 43 (running) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:1238 +0x5d7
  testing.runTests.func1()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:1511 +0xa6
  testing.tRunner()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:1193 +0x202
  testing.runTests()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:1509 +0x612
  testing.(*M).Run()
      /opt/hostedtoolcache/go/1.16.3/x64/src/testing/testing.go:1417 +0x3b3
  main.main()
      _testmain.go:105 +0x356
==================
2021/05/05 21:27:06 httptest.Server blocked in Close after 5 seconds, waiting for connections:
  *net.TCPConn 0xc000088108 127.0.0.1:57546 in state active
--- FAIL: TestEngineUpgrade (60.03s)
    engine_test.go:282: 
        	Error Trace:	engine_test.go:282
        	Error:      	Expected nil, but got: &websocket.CloseError{Code:1006, Text:"unexpected EOF"}
        	Test:       	TestEngineUpgrade
    engine_test.go:197: 
        	Error Trace:	engine_test.go:197
        	            				asm_amd64.s:1371
        	Error:      	Expected nil, but got: &errors.errorString{s:"EOF"}
        	Test:       	TestEngineUpgrade
    testing.go:1092: race detected during execution of test
FAIL

To Reproduce make test. As it's a race condition, it's hard to reproduce it every time.

Expected behavior No race condition.

Environment (please complete the following information):

  • Go version: 1.16
  • Server version: 1.4
  • Client version: 1.4

googollee avatar May 05 '21 21:05 googollee

It is so strange. Into golang version 1.16 does not detected but into 1.15 was detected race. Is broken race condition detector into new go-version?

sshaplygin avatar May 10 '21 01:05 sshaplygin

It could be 1.16 changed the scheduler and make it not so easy to trigger. I always feel that the upgrading process of old code is not a good design. That's why I hope to do a new design directly.

googollee avatar May 10 '21 19:05 googollee

That's why I hope to do a new design directly.

That's good idea

I think maybe add old version 1.13 and 1.14 into github action. What do you think about it?

sshaplygin avatar May 11 '21 06:05 sshaplygin

Just do it.

googollee avatar May 11 '21 17:05 googollee

Hey, Googol! Is it actually? sorry, i don't understand. It is so strange behaviour

sshaplygin avatar May 20 '21 22:05 sshaplygin

I believe it's very hard to fix with the current code. As I said in https://github.com/googollee/go-socket.io/issues/456#issuecomment-837243206, I'd like to design a new way to handle upgrade properly.

googollee avatar May 21 '21 19:05 googollee