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.

Sign up for GitHub

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

Jump to bottom

nsqd: update dependencies #1387

Merged
merged 2 commits into from
Oct 25, 2021
Merged

nsqd: update dependencies #1387

merged 2 commits into from
Oct 25, 2021

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Oct 24, 2021

No description provided.

@ploxiln
Copy link
Member Author

ploxiln commented Oct 24, 2021

I tried putting the lowest supported go version in go.mod, but the result was:

nsqadmin/static.go:10:3: go:embed requires go1.16 or later (-lang was set to go1.14; check go.mod)

(declaring "go 1.16" seems to work fine with previous versions anyway, and "go 1.17" also worked fine I guess ...)

mreiferson
mreiferson reviewed Oct 24, 2021
View reviewed changes
nsqlookupd/lookup_protocol_v1_test.go Outdated
@@ -35,6 +35,8 @@ func testIOLoopReturnsClientErr(t *testing.T, fakeConn test.FakeNetConn) {
opts := NewOptions()
opts.Logger = test.NewTestLogger(t)
opts.LogLevel = LOG_DEBUG
opts.TCPAddress = "127.0.0.1:14163" // avoid possible conflict with other tests
opts.HTTPAddress = "127.0.0.1:14164"
Copy link
Member

Choose a reason for hiding this comment

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

why is this change necessary? does it not handle the :0 "random free port"?

Copy link
Member Author

Choose a reason for hiding this comment

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

this started happening persistently, though just for the go-1.17 test run:

--- FAIL: TestIOLoopReturnsClientErrWhenSendSucceeds (0.00s)
    logger.go:16: INFO: nsqlookupd v1.2.1 (built w/go1.17.2)
    assertions.go:31: lookup_protocol_v1_test.go:40:
        
        	   <nil> (expected)
        
        	!= &errors.errorString{s:"listen (0.0.0.0:4161) failed - listen tcp 0.0.0.0:4161: bind: address already in use"} (actual)

The default ports of 4160/4161 were being used.

does it not handle the :0 "random free port"?

Somehow I had forgotten about that old common unix feature ... will update to do that

@ploxiln ploxiln force-pushed the diskqueue_110 branch 2 times, most recently from e4ecc35 to 5ad79d1 Compare October 24, 2021 21:08
@ploxiln
Copy link
Member Author

ploxiln commented Oct 24, 2021

Updated, squashed a bit.

The race detector caught another unrelated thing (under go-1.14) but a re-test alleviated the problem for now

WARNING: DATA RACE
Write at 0x00c000303d20 by goroutine 135:
  internal/race.Write()
      /usr/local/go/src/internal/race/race.go:41 +0x114
  sync.(*WaitGroup).Wait()
      /usr/local/go/src/sync/waitgroup.go:128 +0x115
  github.com/nsqio/nsq/nsqd.(*NSQD).Exit()
      /__w/nsq/nsq/go/src/github.com/nsqio/nsq/nsqd/nsqd.go:451 +0x364
  github.com/nsqio/nsq/nsqd.TestEmptyCommand()
      /__w/nsq/nsq/go/src/github.com/nsqio/nsq/nsqd/protocol_v2_test.go:447 +0x31d
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1050 +0x1eb

Previous read at 0x00c000303d20 by goroutine 94:
  internal/race.Read()
      /usr/local/go/src/internal/race/race.go:37 +0x1e8
  sync.(*WaitGroup).Add()
      /usr/local/go/src/sync/waitgroup.go:71 +0x1fb
  github.com/nsqio/nsq/internal/util.(*WaitGroupWrapper).Wrap()
      /__w/nsq/nsq/go/src/github.com/nsqio/nsq/internal/util/wait_group_wrapper.go:12 +0x43
  github.com/nsqio/nsq/nsqd.(*NSQD).Main()
      /__w/nsq/nsq/go/src/github.com/nsqio/nsq/nsqd/nsqd.go:253 +0x241
  github.com/nsqio/nsq/nsqd.mustStartNSQD.func1()
      /__w/nsq/nsq/go/src/github.com/nsqio/nsq/nsqd/protocol_v2_test.go:49 +0x38

Goroutine 135 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1095 +0x537
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1339 +0xa6
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1050 +0x1eb
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1337 +0x594
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1252 +0x2ff
  main.main()
      _testmain.go:286 +0x223

Goroutine 94 (running) created at:
  github.com/nsqio/nsq/nsqd.mustStartNSQD()
      /__w/nsq/nsq/go/src/github.com/nsqio/nsq/nsqd/protocol_v2_test.go:48 +0x171
  github.com/nsqio/nsq/nsqd.TestEmptyCommand()
      /__w/nsq/nsq/go/src/github.com/nsqio/nsq/nsqd/protocol_v2_test.go:435 +0x126
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1050 +0x1eb
==================
--- FAIL: TestEmptyCommand (0.00s)
    logger.go:16: INFO: nsqd v1.2.1 (built w/go1.14.15)
    logger.go:16: INFO: ID: 581
    logger.go:16: INFO: NSQ: persisting topic/channel metadata to /tmp/nsq-test-274671300/nsqd.dat
    logger.go:16: INFO: NSQ: closing topics
    logger.go:16: INFO: NSQ: stopping subsystems
    logger.go:16: INFO: LOOKUP: closing
    logger.go:16: INFO: QUEUESCAN: closing
    logger.go:16: INFO: HTTP: listening on 127.0.0.1:45063
    logger.go:16: INFO: HTTP: closing 127.0.0.1:45063
    logger.go:16: INFO: TCP: listening on 127.0.0.1:34935
    logger.go:16: INFO: TCP: closing 127.0.0.1:34935
    logger.go:16: INFO: NSQ: bye
    testing.go:965: race detected during execution of test

mreiferson
mreiferson reviewed Oct 24, 2021
View reviewed changes
nsqlookupd/lookup_protocol_v1_test.go
@@ -35,6 +35,8 @@ func testIOLoopReturnsClientErr(t *testing.T, fakeConn test.FakeNetConn) {
opts := NewOptions()
opts.Logger = test.NewTestLogger(t)
opts.LogLevel = LOG_DEBUG
opts.TCPAddress = "127.0.0.1:0"
opts.HTTPAddress = "127.0.0.1:0"
Copy link
Member

Choose a reason for hiding this comment

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

I guess there are a few tests that don't use mustStartLookupd, which does this automatically as a bootstrap step.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I see. there's also bootstrapNSQCluster() which also does :0 for listening addresses

@mreiferson
Copy link
Member

Not sure what to make of the race detector error given that it's internal to Go source code?

@ploxiln
Copy link
Member Author

ploxiln commented Oct 24, 2021

I think you're supposed to ensure all WaitGroup.Add() is strictly before WaitGroup.Wait()

@mreiferson
Copy link
Member

ready?

@ploxiln
nsqd: update dependencies
60ae5ba 
go-nsq v1.1.0
go-diskqueue v1.1.0
golang/snappy v0.0.4
BurntSushi/toml v0.4.1
@ploxiln
nsqlookupd: less flaky protocol test
e6b949d 
NSQLookupd.New() listens on TCPAddress and HTTPAddress, which
NewOptions() gives default values of 4160 and 4161,
and sometimes this conflicts with other tests running concurrently.
@ploxiln
Copy link
Member Author

ploxiln commented Oct 25, 2021

squashed once more, ready to go

@mreiferson mreiferson changed the title nsqd: update go-diskqueue to v1.1.0 nsqd: update dependencies Oct 25, 2021
@mreiferson mreiferson merged commit 198a884 into nsqio:master Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mreiferson mreiferson mreiferson left review comments

@jehiah jehiah Awaiting requested review from jehiah

Assignees
No one assigned
Labels
chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ploxiln @mreiferson