Mutex protection is insufficient

[dgryski]

The mutex protecting access to GeoIP_last_netmask() is only locked during calls to GetCountry(), but won't any access to the Get* methods potentially mutate GeoIP_last_netmask()? I wonder if this should be a RWMutex instead, and most of the calls (that don't need to get the netmask as a separate call) can use RLock()/RUnlock() and GetCountry() can use the normal writer Lock().

(This is based on a cursory examination of geoip-c-api code; I haven't run this code under the race detector which I'm not sure can peek inside C routines anyway..)

Maybe it's just better to remove the netmask call from the Go API altogether?

[abh]

Nice catch, thank you!

I use the netmask in my application, but I agree for general use it's not that useful. I think if you have an application that's performance sensitive and where this makes a measurable difference then we can look at making it optional. Maybe keep the same call signature, but have a flag or something to disable looking up and returning the netmask (just have it always return 0).

[dgryski]

Somewhat related: libgeoip included a thread-safe API in its 1.5.0 release in October 2012. This would allow the removal of all mutexes from the Go API. The work required is not that large and I'm willing to do it. Most distributions are probably still on earlier versions, although Ubuntu Saucy will include 1.5.1. Should I begin this work?

[abh]

Yeah, it's a pretty reasonable plan – though all my deployments are still on 1.4.x and making a newer libgeoip a requirement to run the app is a little annoying. I wonder if we could make it work with both? It seems like not without a bunch of extra effort and complexity.

Does go 1.2 support statically linking libraries used with cgo? If it was just a matter of having it available when building, maybe that'd be less frustrating. :-/

I wish you'd asked a year or so in the future when the latest RHEL should be out and hopefully have 1.5.x. I guess Ubuntu LTS won't have it until early 2016. :-\

[abh]

I moved this to issue #8.