abh / geoip

GeoIP API for Golang
MIT License
234 stars 66 forks source link

Use thread-safe API in GeoIP 1.5.x #8

Open abh opened 11 years ago

abh commented 11 years ago

(copied from issue #6)

@dryski wrote:

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 commented 11 years ago

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. :-\

dgryski commented 11 years ago

Static linking with cgo has been possible since external linker support was added in 1.1. That seems a reasonable course of action since the other ones I had in my brain (build tags and separate repo, or branch) all looked like they were going to get ugly quickly.

dgryski commented 11 years ago

For the record,

go build -ldflags "-v -linkmode=external \"-extldflags=-static\""

is the appropriate incantation to build an application and force static linking of all C libraries. On my machine, building http://github.com/dgryski/rgip with these flags produces warnings due to the network calls in libgeoip:

/home/dgryski/work/src/cvs/go/src/pkg/net/cgo_unix.go:53: AVERTISSEMENT: Using 'getaddrinfo' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../x86_64-linux-gnu/libGeoIP.a(GeoIP.o): dans la fonction « _GeoIP_lookupaddress »: (.text+0x17f7): AVERTISSEMENT: Using 'gethostbyname_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

That means for deployment, you'll need your build box to be running the same glibc as your production machines.

abh commented 11 years ago

Ugh, messy. I think maybe a 1.5.x branch will be saner.

Or actually – I wonder if we could make the code auto-detect which lib it has and then do the right thing. Are there other differences than doing the mutex or not?

dgryski commented 11 years ago

Yes, all the API calls are named ${foo}_gl and take an extra parameter where the netmask info is dumped. Without ifdef, it makes wrapping the two versions slightly obnoxious, and even with buildtags there is going to be lots of duplicated code.

A slightly less ugly solution might be to write a Go wrapper that ties very closely to the C API and call that from the higher-level API that actually gets exposed to the user. Then the thread-safe one can call the _gl calls directly, and the non-thread-safe ones can do locking and call the non-thread-safe versions.

dgryski commented 10 years ago

Looks like Centos7 and Ubuntu 14.04 (LTS) both include updated GeoIP packages. Ubuntu has 1.6.0, Centos7 has 1.5.0.

abh commented 10 years ago

We can make it optional like in https://github.com/codahale/geoip/blob/master/geoip.go

oryband commented 9 years ago

Any update on this issue?