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

Concurrent map access panic in acceptor #436

Closed
smulube opened this issue May 13, 2021 · 3 comments · Fixed by #449
Closed

Concurrent map access panic in acceptor #436

smulube opened this issue May 13, 2021 · 3 comments · Fixed by #449

Comments

@smulube
Copy link
Contributor

smulube commented May 13, 2021

Hello we are currently using QuickFIXGo in a UAT environment as an acceptor and we have been seeing intermittent panics related to concurrent map accesses within acceptor.go. Unfortunately I don't have a copy of the stack trace to hand but what I suspect is happening is related to concurrent modifications of the maps within the Acceptor struct (sessions which is a map[SessionID]*session and sessionAddr which is a map[SessionID]net.Addr).

Examples of the maps being read or modified inside a spawned goroutine I believe can be seen here: https://github.com/quickfixgo/quickfix/blob/master/acceptor.go#L291 or https://github.com/quickfixgo/quickfix/blob/master/acceptor.go#L307

We have created a fork of the library in which we've added a sync.RWMutex to Acceptor and we are now protecting any reads or modifications to either the sessions or sessionAddr maps, and since making this change we have not seen any repeat of the problem.

Would this change be useful to submit as a pull request, or is there some reason why those map modifications weren't already being guarded by a mutex that we are missing? It's possible that the fact that we haven't seen repeat occurrences is not related to our changes, but would be happy to send it over as a pull request if you think it would be useful.

thanks

@letian0805
Copy link
Contributor

abcd695

@smulube
Copy link
Contributor Author

smulube commented Jun 1, 2021

thanks, yep we have a similar fix in our branch but would like to get this merged into the main line if possible so maybe I will open that PR to try and get that process started.

@letian0805
Copy link
Contributor

I have opened a PR to fix it and another issue.
#432

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

Successfully merging a pull request may close this issue.

2 participants