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

Refactor access with proper mutex guard #258

Closed
influx6 opened this issue Aug 16, 2017 · 4 comments
Closed

Refactor access with proper mutex guard #258

influx6 opened this issue Aug 16, 2017 · 4 comments
Assignees

Comments

@influx6
Copy link
Contributor

influx6 commented Aug 16, 2017

Currently we have in geth/node/manager.go (see https://github.com/status-im/status-go/blob/develop/geth/node/manager.go#L35-L45), where we have both intention in code to guard synchronous access to most fields through the embedded mutex.

Problem

But we currently have in https://github.com/status-im/status-go/blob/develop/geth/node/manager.go#L69-L71 and in many other areas within code where access to NodeManager.node is initially accessed without mutex guard to ensure synch access.

Issues

We could later face issues with race conditions on access to the NodeManager.node field and other fields that maybe concurrently reacehed directly or indirectly.

Fix

We need to refactor this package to ensure proper synchronouse access to the NodeManager.nodeand other fields as intended, else re-arrange NodeManager struct fields to ensure quick intention has to fields which mutex guards.

@JekaMas
Copy link
Contributor

JekaMas commented Oct 12, 2017

Actually we already got a lot of dara races in node/manager.go, node/node.go and node/utils.go.
I think we should turn on a data race detector in ci.

@tiabc
Copy link
Contributor

tiabc commented Oct 12, 2017

@JekaMas #50

@JekaMas
Copy link
Contributor

JekaMas commented Oct 18, 2017

I done most of 403 to complite this task:

  • NodeManager is simplified
  • Some e2e tests are added
  • There are no race conditions when running unit tests with -race.
  • NodeManager is thread-safe

@mandrigin
Copy link
Contributor

@divan @adambabik closing in favour of status-im/swarms#63

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

No branches or pull requests

4 participants