grid-x / modbus

BSD 3-Clause "New" or "Revised" License
78 stars 26 forks source link

Fix races in unit-tests #82

Closed srebhan closed 7 months ago

srebhan commented 7 months ago

Running go test -v -race will report two races, one for TestSerialCloseIdle and one for TestTCPTransporter (see excerpt below). This PR fixes the races in both tests.

=== RUN   TestSerialCloseIdle
==================
WARNING: DATA RACE
Read at 0x00c000244088 by goroutine 42:
  github.com/grid-x/modbus.TestSerialCloseIdle()
      /home/sven/Development/InfluxData/modbus/serial_test.go:33 +0x255
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x261
  testing.(*T).Run.func1()
      /usr/lib/go/src/testing/testing.go:1648 +0x44

Previous write at 0x00c000244088 by goroutine 44:
  github.com/grid-x/modbus.(*nopCloser).Close()
      /home/sven/Development/InfluxData/modbus/serial_test.go:17 +0x2f
  github.com/grid-x/modbus.(*serialPort).close()
      /home/sven/Development/InfluxData/modbus/serial.go:66 +0x282
  github.com/grid-x/modbus.(*serialPort).closeIdle()
      /home/sven/Development/InfluxData/modbus/serial.go:100 +0x23f
  github.com/grid-x/modbus.(*serialPort).closeIdle-fm()
      <autogenerated>:1 +0x33

Goroutine 42 (running) created at:
  testing.(*T).Run()
      /usr/lib/go/src/testing/testing.go:1648 +0x845
  testing.runTests.func1()
      /usr/lib/go/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x261
  testing.runTests()
      /usr/lib/go/src/testing/testing.go:2052 +0x8ad
  testing.(*M).Run()
      /usr/lib/go/src/testing/testing.go:1925 +0xcd7
  main.main()
      _testmain.go:91 +0x2bd

Goroutine 44 (finished) created at:
  time.goFunc()
      /usr/lib/go/src/time/sleep.go:176 +0x44
==================
==================
WARNING: DATA RACE
Read at 0x00c00026a080 by goroutine 42:
  github.com/grid-x/modbus.TestSerialCloseIdle()
      /home/sven/Development/InfluxData/modbus/serial_test.go:33 +0x27c
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x261
  testing.(*T).Run.func1()
      /usr/lib/go/src/testing/testing.go:1648 +0x44

Previous write at 0x00c00026a080 by goroutine 44:
  github.com/grid-x/modbus.(*serialPort).close()
      /home/sven/Development/InfluxData/modbus/serial.go:67 +0x28f
  github.com/grid-x/modbus.(*serialPort).closeIdle()
      /home/sven/Development/InfluxData/modbus/serial.go:100 +0x23f
  github.com/grid-x/modbus.(*serialPort).closeIdle-fm()
      <autogenerated>:1 +0x33

Goroutine 42 (running) created at:
  testing.(*T).Run()
      /usr/lib/go/src/testing/testing.go:1648 +0x845
  testing.runTests.func1()
      /usr/lib/go/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x261
  testing.runTests()
      /usr/lib/go/src/testing/testing.go:2052 +0x8ad
  testing.(*M).Run()
      /usr/lib/go/src/testing/testing.go:1925 +0xcd7
  main.main()
      _testmain.go:91 +0x2bd

Goroutine 44 (finished) created at:
  time.goFunc()
      /usr/lib/go/src/time/sleep.go:176 +0x44
==================
    testing.go:1465: race detected during execution of test
--- FAIL: TestSerialCloseIdle (0.15s)

and

=== RUN   TestTCPTransporter
==================
WARNING: DATA RACE
Read at 0x00c0001360e0 by goroutine 48:
  github.com/grid-x/modbus.TestTCPTransporter()
      /home/sven/Development/InfluxData/modbus/tcpclient_test.go:86 +0x5cd
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x261
  testing.(*T).Run.func1()
      /usr/lib/go/src/testing/testing.go:1648 +0x44

Previous write at 0x00c0001360e0 by goroutine 52:
  github.com/grid-x/modbus.(*tcpTransporter).close()
      /home/sven/Development/InfluxData/modbus/tcpclient.go:395 +0x275
  github.com/grid-x/modbus.(*tcpTransporter).closeIdle()
      /home/sven/Development/InfluxData/modbus/tcpclient.go:411 +0x22e
  github.com/grid-x/modbus.(*tcpTransporter).closeIdle-fm()
      <autogenerated>:1 +0x33

Goroutine 48 (running) created at:
  testing.(*T).Run()
      /usr/lib/go/src/testing/testing.go:1648 +0x845
  testing.runTests.func1()
      /usr/lib/go/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x261
  testing.runTests()
      /usr/lib/go/src/testing/testing.go:2052 +0x8ad
  testing.(*M).Run()
      /usr/lib/go/src/testing/testing.go:1925 +0xcd7
  main.main()
      _testmain.go:91 +0x2bd

Goroutine 52 (finished) created at:
  time.goFunc()
      /usr/lib/go/src/time/sleep.go:176 +0x44
==================
    testing.go:1465: race detected during execution of test
--- FAIL: TestTCPTransporter (0.15s)
andig commented 7 months ago

OT: @srebhan if you're interested in using this module in concurrent situations I'd appreciate a review of https://github.com/grid-x/modbus/pull/70. Would be great to get this proposal unstuck.

srebhan commented 7 months ago

@andig thanks for the hint, but currently we define multiple clients if used concurrently (though not over serial line) and are not planning to change this as of now.

andig commented 7 months ago

OT: multiple clients sharing the same underlying network connection to the same modbus server?

srebhan commented 7 months ago

@andig the code is here. You can define multiple requests in a plugin instance, those will be done sequentially. Additionally it is possible to have multiple of those plugins running concurrently and then they share nothing, i.e. each instance will open an own connection to the configured endpoint.

We can chat on out slack if you are interested in more details to avoid cluttering this PR... ;-)

srebhan commented 7 months ago

@andig is there anything I should do in this commit to get this merged?

andig commented 7 months ago

I can‘t speak for @grid-x, sorry

srebhan commented 7 months ago

@hsteidl anything I can do to get this merged?