search

googollee / go-socket.io

socket.io library for golang, a realtime application framework.
Other
5.65k stars 830 forks source link

Fix race condition. #456

Open googollee opened 3 years ago

googollee commented 3 years ago

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/testify@v1.5.1/require/require.go:955 +0x104
  github.com/stretchr/testify/require.(*Assertions).Nil()
      /home/runner/go/pkg/mod/github.com/stretchr/testify@v1.5.1/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/testify@v1.5.1/require/require.go:955 +0x104
  github.com/stretchr/testify/require.(*Assertions).Nil()
      /home/runner/go/pkg/mod/github.com/stretchr/testify@v1.5.1/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):

sshaplygin commented 3 years ago

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?

googollee commented 3 years ago

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.

sshaplygin commented 3 years ago

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?

googollee commented 3 years ago

Just do it.

sshaplygin commented 3 years ago

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

googollee commented 3 years ago

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.