Skip to content

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

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

Open
abh opened this issue Sep 14, 2013 · 8 comments
Open

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

abh opened this issue Sep 14, 2013 · 8 comments

Comments

@abh
Copy link
Owner

abh commented Sep 14, 2013

(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
Copy link
Owner Author

abh commented Sep 14, 2013

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
Copy link
Contributor

dgryski commented Sep 15, 2013

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
Copy link
Contributor

dgryski commented Sep 15, 2013

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
Copy link
Owner Author

abh commented Sep 15, 2013

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
Copy link
Contributor

dgryski commented Sep 16, 2013

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
Copy link
Contributor

dgryski commented Aug 13, 2014

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

@abh
Copy link
Owner Author

abh commented Sep 29, 2014

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

@oryband
Copy link

oryband commented Jan 1, 2015

Any update on this issue?

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

No branches or pull requests

3 participants