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

nsqlookupd: synchronize goroutines to avoid t.Log data races #1190

Merged
merged 1 commit into from
Sep 20, 2019

Conversation

benjsto
Copy link
Contributor

@benjsto benjsto commented Sep 18, 2019

Fixes the data race seen here: #1157 by providing a mechanism for all handler goroutines in nsqlookupd to exit before tests finish.

I'm obviously new here so let me know if there are better/simpler ways to accomplish this.

@benjsto benjsto changed the title nsqlookupd: synchronize goroutines to avoid t.Log races nsqlookupd: synchronize goroutines to avoid t.Log data races Sep 18, 2019
@benjsto benjsto force-pushed the benjsto/go-1-12-tests branch from e83b1e6 to 1837898 Compare September 19, 2019 02:04
nsqlookupd/tcp.go Outdated Show resolved Hide resolved
@ploxiln
Copy link
Member

ploxiln commented Sep 19, 2019

worth noting: nsqd also uses TCPServer, and already closes all connections via Topic.Close() -> Channel.Close() -> Consumer.Close()

I think this is good. Please squash commits together before merge.

@benjsto benjsto force-pushed the benjsto/go-1-12-tests branch from 8f7352e to dbc2b9c Compare September 19, 2019 22:39
@benjsto
Copy link
Contributor Author

benjsto commented Sep 19, 2019

I assumed something like that was going on for nsqd's use of TCPServer but hadn't dug into it. Thanks for clarifying. Thanks for your help on this - I've removed that unnecessary Delete and squashed and rebased.

@benjsto benjsto force-pushed the benjsto/go-1-12-tests branch from dbc2b9c to 61de19e Compare September 19, 2019 22:51
@ploxiln ploxiln merged commit 09b06de into nsqio:master Sep 20, 2019
ploxiln added a commit to ploxiln/nsq that referenced this pull request Oct 15, 2019
Consumer connections are closed when topics are closed,
but tcp-protocol publish connections are not,
so add tcpServer.CloseAll().

problem introduced by nsqio#1190
ploxiln added a commit to ploxiln/nsq that referenced this pull request Oct 15, 2019
Consumer connections are closed when topics are closed,
but tcp-protocol publish connections are not,
so add tcpServer.CloseAll().

problem introduced by nsqio#1190
@mreiferson mreiferson added the bug label Jun 14, 2020
suhailpatel added a commit to monzo/nsq that referenced this pull request Oct 7, 2021
This commit picks nsqio#1190 and backports
it to this version of NSQ to avoid data races in t.Log flagged up by the
race detector
suhailpatel added a commit to monzo/nsq that referenced this pull request Dec 16, 2021
This commit picks nsqio#1190 and backports
it to this version of NSQ to avoid data races in t.Log flagged up by the
race detector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants