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
The global queries is accessed by multiple goroutines, why not lock for it? #1
Comments
Because there's no reason to do so. A query entry is written only in one place, and wherever it's accessed it has to be there for the access to succeed, ergo it doesn't need locking. Unless you can find a case where you can hack it to blow up :) Feel free to do so, it has been running smoothly 24h/7d for > 2 years now, but I never claimed it's |
I think I've found a place where it blows up. Some information about the circumstances:
Here there is the trace from the crash:
My advice is to always lock around maps if there are more than one goroutines which use them. I do suspect my word does not weight much on the subject, but Russ Cox's probably does. Also, there is this excellent article on the golang blog. I can see how easily a writing in the Judging by your travis file I suspect that you've compiled your code with go1.3. So it is not a surprise that it has never crashed for you. For this version, GOMAXPROCS was still 1 by default which meant that no concurrent access was possible whatsoever. If your lovely project fits my needs I would probably make a pull request which fixes this small issues. |
@ironsmile :D Thanks for a great issue submission. Indeed my first impression was Locking on both sides sounds like the proper solution. I'll very much welcome a PR for this, but this being now a real issue I'll fix it anyway whenever I get some free time and spare brain cycles. Thanks again! |
@jannson and now it seems you've been onto something. Seems I've taken this too lightly, which indeed is my bad. |
@jannson @ironsmile Hey guys, PRed a fix -> #3 but can't really test it with my RPi setup. Would prefer to merge after someone actually tested it in a reasonable environment. It'd be very much appreciated if you guys could give it a try :) |
Will do! Would be one of the first things I do once I am back from a short codeless vacation :) |
Like the code here: https://github.com/drbig/adhole/blob/master/adhole/main.go#L333
Or here: https://github.com/drbig/adhole/blob/master/adhole/main.go#L341
The text was updated successfully, but these errors were encountered: