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

Protect calls to GeoIP calls with our mutex #9

Merged
merged 3 commits into from
Oct 15, 2013
Merged

Protect calls to GeoIP calls with our mutex #9

merged 3 commits into from
Oct 15, 2013

Conversation

dgryski
Copy link
Contributor

@dgryski dgryski commented Oct 14, 2013

Tracing through the call stack we have a few calls that end up calling _GeoIP_seek_record deep in the callstack which clobbers our global netmask. So, even though we don't query for the netmask, to prevent some other goroutine from getting the wrong answer we need to protect these calls too.

Tracing through the call stack we have:
GeoIP_region_by_addr
which calls _get_region
which calls GeoIP_assign_region_by_inetaddr
which calls_GeoIP_seek_record
which clobbers our global netmask.
@abh
Copy link
Owner

abh commented Oct 15, 2013

Hi Damian,

Thank you. It'll be nice when RHEL and the LTS Ubuntu have caught up to the 1.5.x release ... [ eh, I wrote "your" before; I was thinking you worked for MaxMind -- who else would bother to dig around in libgeoip enough to notice this? ]

What was the reason for the "embed the mutex" change? Should the mutex be visible/usable by users of the package? What's the use case?

Ask

@dgryski
Copy link
Contributor Author

dgryski commented Oct 15, 2013

Arggh, I forgot that would make it externally visible. That particular commit can be ditched.

@dgryski
Copy link
Contributor Author

dgryski commented Oct 15, 2013

Commit removed and pull request updated -- you should be fine to just merge now.

abh added a commit that referenced this pull request Oct 15, 2013
Protect calls to GeoIP calls with our mutex
@abh abh merged commit da13074 into abh:master Oct 15, 2013
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 this pull request may close these issues.

2 participants