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

Add Mutex on Top of p2p Feeds Map #321

Merged
merged 10 commits into from
Jul 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions .travis-bazelrc
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
startup --host_jvm_args=-Xmx500m --host_jvm_args=-Xms500m

# Disable sandboxing since it may fail inside of Travis' containers because the
# mount system call is not permitted.
build --spawn_strategy=standalone --genrule_strategy=standalone

# Remote caching over Google Cloud Storage
# TODO(#282): Enable remote caching/execution
#build:remote --remote_http_cache=https://storage.googleapis.com/prysmatic-bazel-cache
Expand All @@ -12,7 +8,7 @@ build --spawn_strategy=standalone --genrule_strategy=standalone
# Set some build options for travis container.
build --local_resources=1536,1.5,0.5
build --noshow_progress
build --verbose_failures
build --verbose_failures
build --sandbox_debug
build --test_output=errors
build --flaky_test_attempts=5
6 changes: 4 additions & 2 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
name = "io_bazel_rules_go",
urls = ["https://github.com/bazelbuild/rules_go/releases/download/0.13.0/rules_go-0.13.0.tar.gz"],
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed ?

Copy link
Member

Choose a reason for hiding this comment

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

+1, I think this was to fix the mac issue we ran into a month back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@terenc3t about the reasons for this change see #321 (comment), if 0.13.0 is required i'lll remove the concurrent write unit test and the disable the race detector again, it can be added once 0.13.0 (or a higher version) supports it.

Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment stating that rules_go should not be updated until that specific bug has been resolved.

The Mac issue that others were referring to had to do with bazel itself, not these rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, added comment with a link to the bug.

sha256 = "ba79c532ac400cefd1859cbc8a9829346aa69e3b99482cd5a54432092cbc3933",
# in order to be able to enable race detection we need to use a version
# < 0.13.0 until this bug is fixed: https://github.com/bazelbuild/rules_go/issues/1592
urls = ["https://github.com/bazelbuild/rules_go/releases/download/0.12.1/rules_go-0.12.1.tar.gz"],
sha256 = "8b68d0630d63d95dacc0016c3bb4b76154fe34fca93efd65d1c366de3fcb4294",
)

http_archive(
Expand Down
11 changes: 11 additions & 0 deletions shared/p2p/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,14 @@ go_test(
"@com_github_sirupsen_logrus//hooks/test:go_default_library",
],
)

# by default gazelle tries to add all the test files to the first
# go_test; we want to treat feed_concurrent_test.go differently
# gazelle:exclude feed_concurrent_test.go

go_test(
name = "go_feed_concurrent_write_test",
srcs = ["feed_concurrent_test.go"],
embed = [":go_default_library"],
race = "on",
)
3 changes: 3 additions & 0 deletions shared/p2p/feed.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,11 @@ func (s *Server) Feed(msg interface{}) *event.Feed {
t = reflect.TypeOf(msg)
}

s.mutex.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Please add defer s.mutex.Unlock() on the next line and remove the other unlock. This makes it more clear that you’re releasing the locked resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

defer s.mutex.Unlock()
if s.feeds[t] == nil {
s.feeds[t] = new(event.Feed)
}

return s.feeds[t]
}
14 changes: 14 additions & 0 deletions shared/p2p/feed_concurrent_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package p2p

import "testing"

func TestFeed_ConcurrentWrite(t *testing.T) {
s, err := NewServer()
if err != nil {
t.Fatalf("could not create server %v", err)
}

for i := 0; i < 5; i++ {
go s.Feed("a")
}
}
3 changes: 3 additions & 0 deletions shared/p2p/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ package p2p
import (
"context"
"reflect"
"sync"

"github.com/ethereum/go-ethereum/event"
"github.com/golang/protobuf/proto"
Expand All @@ -33,6 +34,7 @@ type Sender interface {
type Server struct {
ctx context.Context
cancel context.CancelFunc
mutex *sync.Mutex
feeds map[reflect.Type]*event.Feed
host host.Host
gsub *floodsub.PubSub
Expand Down Expand Up @@ -60,6 +62,7 @@ func NewServer() (*Server, error) {
feeds: make(map[reflect.Type]*event.Feed),
host: host,
gsub: gsub,
mutex: &sync.Mutex{},
}, nil
}

Expand Down
2 changes: 2 additions & 0 deletions shared/p2p/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"io/ioutil"
"reflect"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -82,6 +83,7 @@ func TestSubscribeToTopic(t *testing.T) {
gsub: gsub,
host: h,
feeds: make(map[reflect.Type]*event.Feed),
mutex: &sync.Mutex{},
}

feed := s.Feed(pb.CollationBodyRequest{})
Expand Down