Skip to content

Conversation

@srebhan
Copy link
Contributor

@srebhan srebhan commented Feb 1, 2024

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
Copy link
Contributor

andig commented Feb 1, 2024

OT: @srebhan if you're interested in using this module in concurrent situations I'd appreciate a review of #70. Would be great to get this proposal unstuck.

@srebhan
Copy link
Contributor Author

srebhan commented Feb 1, 2024

@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
Copy link
Contributor

andig commented Feb 1, 2024

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

@srebhan
Copy link
Contributor Author

srebhan commented Feb 1, 2024

@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
Copy link
Contributor Author

srebhan commented Feb 12, 2024

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

@andig
Copy link
Contributor

andig commented Feb 12, 2024

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

@srebhan
Copy link
Contributor Author

srebhan commented Feb 12, 2024

@hsteidl anything I can do to get this merged?

Copy link
Contributor

@hnicolaysen hnicolaysen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! The test fix looks good to me. Thanks for sharing!

@hnicolaysen hnicolaysen merged commit 0d4922f into grid-x:master Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants