Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data Race in SecureChannel (yet another) #231

Closed
alexbrdn opened this issue Jun 28, 2019 · 2 comments · Fixed by #232
Closed

Data Race in SecureChannel (yet another) #231

alexbrdn opened this issue Jun 28, 2019 · 2 comments · Fixed by #232
Labels
bug Something isn't working
Milestone

Comments

@alexbrdn
Copy link
Collaborator

With #227 applied and under the same conditions as in #225, I get the following data race in SecureChannel:

==================
WARNING: DATA RACE
Write at 0x00c00013806c by goroutine 17:
  github.com/gopcua/opcua/uasc.(*SecureChannel).Receive()
      .../opcua/uasc/secure_channel.go:354 +0x251
  github.com/gopcua/opcua.(*Client).monitorChannel()
      .../opcua/client.go:191 +0xbe

Previous write at 0x00c00013806c by main goroutine:
  github.com/gopcua/opcua/uasc.(*SecureChannel).sendAsyncWithTimeout()
      .../opcua/uasc/secure_channel.go:176 +0x481
  github.com/gopcua/opcua/uasc.(*SecureChannel).SendWithTimeout()
      .../opcua/uasc/secure_channel.go:154 +0xe3
  github.com/gopcua/opcua.(*Client).sendWithTimeout()
      .../opcua/client.go:439 +0xd3
  github.com/gopcua/opcua.(*Client).closeSession()
      .../opcua/client.go:428 +0x1bd
  github.com/gopcua/opcua.(*Client).CloseSession()
      .../opcua/client.go:396 +0x7c
  github.com/gopcua/opcua.(*Client).Close()
      .../opcua/client.go:211 +0x43
  main.main()
      .../opcua/examples/subscribe/subscribe.go:107 +0x119a

Goroutine 17 (running) created at:
  github.com/gopcua/opcua.(*Client).Dial()
      .../opcua/client.go:174 +0x2fd
  github.com/gopcua/opcua.(*Client).Connect()
      .../opcua/client.go:139 +0x17b
  main.main()
      .../opcua/examples/subscribe/subscribe.go:60 +0xa99
==================
Found 1 data race(s)

I guess this line from #211 was the last addition leading to data race: s.cfg.RequestID = reqid.

I don't think that Receive(), when executed in client, should change s.cfg.RequestID. Whether it should do the same when running as server, I'm not sure. @dwhutchison, help needed.

@dwhutchison
Copy link
Collaborator

You're right; that was for the server and shouldn't execute for clients. I put that in there so the server responses echo the request ID back to the client. Feel free to delete it for now and I'll find a better way to do this in the server code

alexbrdn added a commit that referenced this issue Jul 1, 2019
@alexbrdn alexbrdn added the bug Something isn't working label Jul 3, 2019
@alexbrdn
Copy link
Collaborator Author

alexbrdn commented Jul 4, 2019

Addendum for the sake of completeness.

The same line of code sometimes results in a different problem. Say, the client sends two requests with ids 1 and 2. The server responds to 1 while 2 is still waiting. s.cfg.RequestId is set to 1. Client tries to send a new request, which gets assigned the id 2 again and the client bails at the safety check "error: duplicate handler registration"

@magiconair magiconair added this to the v0.1.2 milestone Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants