You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/05 07:22:09 UTC
[GitHub] [pulsar-client-go] equanz opened a new pull request #581: Fix data race issue in ServiceNameResolver
equanz opened a new pull request #581:
URL: https://github.com/apache/pulsar-client-go/pull/581
### Motivation
When running test command, the data race warning occurred in my CI environment.
```
==================
WARNING: DATA RACE
Read at 0x00c0007936c0 by goroutine 472:
github.com/apache/pulsar-client-go/pulsar/internal.(*pulsarServiceNameResolver).ResolveHost()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/internal/service_name_resolver.go:66 +0x167
github.com/apache/pulsar-client-go/pulsar/internal.(*rpcClient).RequestToAnyBroker()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/internal/rpc_client.go:82 +0x70
github.com/apache/pulsar-client-go/pulsar/internal.(*lookupService).GetPartitionedTopicMetadata()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/internal/lookup_service.go:183 +0x2ab
github.com/apache/pulsar-client-go/pulsar.(*client).TopicPartitions()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/client_impl.go:184 +0xfd
github.com/apache/pulsar-client-go/pulsar.(*producer).internalCreatePartitionsProducers()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/producer_impl.go:161 +0xcf
github.com/apache/pulsar-client-go/pulsar.(*producer).runBackgroundPartitionDiscovery.func1()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/producer_impl.go:148 +0x104
Previous write at 0x00c0007936c0 by goroutine 475:
sync/atomic.StoreInt32()
/opt/go/src/runtime/race_amd64.s:242 +0xb
github.com/apache/pulsar-client-go/pulsar/internal.(*pulsarServiceNameResolver).ResolveHost()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/internal/service_name_resolver.go:67 +0x1b1
github.com/apache/pulsar-client-go/pulsar/internal.(*rpcClient).RequestToAnyBroker()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/internal/rpc_client.go:82 +0x70
github.com/apache/pulsar-client-go/pulsar/internal.(*lookupService).Lookup()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/internal/lookup_service.go:109 +0x408
github.com/apache/pulsar-client-go/pulsar.(*partitionConsumer).grabConn()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/consumer_partition.go:897 +0x102
github.com/apache/pulsar-client-go/pulsar.newPartitionConsumer()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/consumer_partition.go:179 +0xc7e
github.com/apache/pulsar-client-go/pulsar.newReader()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/reader_impl.go:105 +0x96c
github.com/apache/pulsar-client-go/pulsar.(*client).CreateReader()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/client_impl.go:170 +0xd7
github.com/apache/pulsar-client-go/pulsar.TestReaderWithMultiHosts()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/reader_test.go:620 +0x6d4
testing.tRunner()
/opt/go/src/testing/testing.go:1193 +0x202
Goroutine 472 (running) created at:
github.com/apache/pulsar-client-go/pulsar.(*producer).runBackgroundPartitionDiscovery()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/producer_impl.go:140 +0x125
github.com/apache/pulsar-client-go/pulsar.newProducer()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/producer_impl.go:128 +0x6ce
github.com/apache/pulsar-client-go/pulsar.(*client).CreateProducer()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/client_impl.go:153 +0xc4
github.com/apache/pulsar-client-go/pulsar.TestReaderWithMultiHosts()
/sd/workspace/src/github.com/apache/pulsar-client-go/pulsar/reader_test.go:603 +0x295
testing.tRunner()
/opt/go/src/testing/testing.go:1193 +0x202
Goroutine 475 (running) created at:
testing.(*T).Run()
/opt/go/src/testing/testing.go:1238 +0x5d7
testing.runTests.func1()
/opt/go/src/testing/testing.go:1511 +0xa6
testing.tRunner()
/opt/go/src/testing/testing.go:1193 +0x202
testing.runTests()
/opt/go/src/testing/testing.go:1509 +0x612
testing.(*M).Run()
/opt/go/src/testing/testing.go:1417 +0x3b3
main.main()
_testmain.go:419 +0x356
==================
time="2021-07-14T11:08:33+09:00" level=info msg="[Connecting to broker]" remote_addr="pulsar://localhost:6600"
time="2021-07-14T11:08:33+09:00" level=info msg="[Connected consumer]" consumerID=1 name= subscription=reader-psjga topic=my-topic-508454231
time="2021-07-14T11:08:33+09:00" level=info msg="[Created consumer]" consumerID=1 name= subscription=reader-psjga topic=my-topic-508454231
time="2021-07-14T11:08:33+09:00" level=info msg="Closing consumer=1" consumerID=1 name= subscription=reader-psjga topic=my-topic-508454231
time="2021-07-14T11:08:33+09:00" level=info msg="[Closed consumer]" consumerID=1 name= subscription=reader-psjga topic=my-topic-508454231
time="2021-07-14T11:08:36+09:00" level=warning msg="[Failed to connect to broker.]" error="dial tcp 127.0.0.1:6600: connect: connection refused" remote_addr="pulsar://localhost:6600"
time="2021-07-14T11:08:36+09:00" level=info msg="[Connection closed]" remote_addr="pulsar://localhost:6600"
time="2021-07-14T11:08:36+09:00" level=info msg="[Closing producer]" producerID=1 producer_name=standalone-0-183 topic="persistent://public/default/my-topic-508454231"
time="2021-07-14T11:08:36+09:00" level=info msg="[Closed producer]" producerID=1 producer_name=standalone-0-183 topic="persistent://public/default/my-topic-508454231"
time="2021-07-14T11:08:36+09:00" level=info msg="[Connection closed]" local_addr="127.0.0.1:40190" remote_addr="pulsar://localhost:6650"
--- FAIL: TestReaderWithMultiHosts (3.75s)
testing.go:1092: race detected during execution of test
```
I think this issue is not critical, but I'd like to fix this issue.
### Modifications
* Add mutex in ServiceNameResolver to avoid race condition of `r.CurrentIndex`
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
### Documentation
- Does this pull request introduce a new feature? (no)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] equanz commented on a change in pull request #581: Fix data race issue in ServiceNameResolver
Posted by GitBox <gi...@apache.org>.
equanz commented on a change in pull request #581:
URL: https://github.com/apache/pulsar-client-go/pull/581#discussion_r685692859
##########
File path: pulsar/internal/service_name_resolver.go
##########
@@ -98,8 +106,9 @@ func (r *pulsarServiceNameResolver) UpdateServiceURL(u *url.URL) error {
r.AddressList = addresses
r.ServiceURL = u
r.ServiceURI = uri
- rand.Seed(time.Now().Unix()) // initialize global pseudo random generator
- atomic.StoreInt32(&r.CurrentIndex, int32(rand.Intn(len(addresses))))
+ r.mutex.Lock()
Review comment:
In this PR, I only mentioned `CurrentIndex`. However I think your comment is correct, so I'll address your comment.
##########
File path: pulsar/internal/service_name_resolver.go
##########
@@ -42,6 +42,12 @@ type pulsarServiceNameResolver struct {
ServiceURL *url.URL
CurrentIndex int32
AddressList []*url.URL
Review comment:
> Not sure why all these are public
I think so too.
I won't change the visibility in this PR, but we might want to do so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] cckellogg merged pull request #581: Fix data race issue in ServiceNameResolver
Posted by GitBox <gi...@apache.org>.
cckellogg merged pull request #581:
URL: https://github.com/apache/pulsar-client-go/pull/581
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request #581: Fix data race issue in ServiceNameResolver
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #581:
URL: https://github.com/apache/pulsar-client-go/pull/581#discussion_r684330780
##########
File path: pulsar/internal/service_name_resolver.go
##########
@@ -63,6 +66,8 @@ func (r *pulsarServiceNameResolver) ResolveHost() (*url.URL, error) {
if len(r.AddressList) == 1 {
return r.AddressList[0], nil
}
+ r.mutex.Lock()
+ defer r.mutex.Unlock()
Review comment:
We should move
```
r.mutex.Lock()
defer r.mutex.Unlock()
```
To the top since the func `UpdateServiceURL` below can alter the AddressList
##########
File path: pulsar/internal/service_name_resolver.go
##########
@@ -98,8 +106,9 @@ func (r *pulsarServiceNameResolver) UpdateServiceURL(u *url.URL) error {
r.AddressList = addresses
r.ServiceURL = u
r.ServiceURI = uri
- rand.Seed(time.Now().Unix()) // initialize global pseudo random generator
- atomic.StoreInt32(&r.CurrentIndex, int32(rand.Intn(len(addresses))))
+ r.mutex.Lock()
Review comment:
We should move the
```
r.mutex.Lock()
defer r.mutex.Unlock()
```
to under `uri, err := NewPulsarServiceURIFromURL(u)`
Since state other piece of state are being changed.
##########
File path: pulsar/internal/service_name_resolver.go
##########
@@ -42,6 +42,12 @@ type pulsarServiceNameResolver struct {
ServiceURL *url.URL
CurrentIndex int32
AddressList []*url.URL
Review comment:
Not sure why all these are public while the struct is private but that can be corrected in another MR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] equanz edited a comment on pull request #581: Fix data race issue in ServiceNameResolver
Posted by GitBox <gi...@apache.org>.
equanz edited a comment on pull request #581:
URL: https://github.com/apache/pulsar-client-go/pull/581#issuecomment-894019797
@cckellogg
Addressed your comments.
PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] equanz commented on pull request #581: Fix data race issue in ServiceNameResolver
Posted by GitBox <gi...@apache.org>.
equanz commented on pull request #581:
URL: https://github.com/apache/pulsar-client-go/pull/581#issuecomment-894019797
@cckellogg
Addressed your comment.
PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request #581: Fix data race issue in ServiceNameResolver
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #581:
URL: https://github.com/apache/pulsar-client-go/pull/581#discussion_r683457449
##########
File path: pulsar/internal/service_name_resolver.go
##########
@@ -63,6 +66,8 @@ func (r *pulsarServiceNameResolver) ResolveHost() (*url.URL, error) {
if len(r.AddressList) == 1 {
return r.AddressList[0], nil
}
+ r.mutex.Lock()
+ defer r.mutex.Unlock()
Review comment:
If we are using a mutex now there is no need for the atomic store. Same for the code below.
##########
File path: pulsar/internal/service_name_resolver.go
##########
@@ -99,6 +104,8 @@ func (r *pulsarServiceNameResolver) UpdateServiceURL(u *url.URL) error {
r.ServiceURL = u
r.ServiceURI = uri
rand.Seed(time.Now().Unix()) // initialize global pseudo random generator
Review comment:
This is kind of outside of the change but this should be done in the init function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org