GitSquared / node-geolite2-redist

Redistribution of MaxMind GeoLite2 GeoIP databases as an npm library
https://gitsquared.github.io/node-geolite2-redist/
Other
72 stars 15 forks source link

lookup.close missing in maxmind #3

Closed AnandChowdhary closed 4 years ago

AnandChowdhary commented 4 years ago

In the docs, it says we can use lookup.close(); to "close event loop", but I can't find the documentation of any close method in maxmind

GitSquared commented 4 years ago

This is redist-specific, it stops the internal update-checker, which occupies a spot in node's event loop.

In some situations depending on where you're using the lib, you might skip this step.

Emptying the event loop is needed only when:

AnandChowdhary commented 4 years ago

Understood, thanks!

So does this mean that if I call lookup.exit() already after setting it up, I can continue to use lookup.get() but it won't auto-update anymore, if that's my specific use case?

GitSquared commented 4 years ago

Assuming you meant lookup.close(): To comply with the license you need to let the library self-update. Since the geolite2.open() function is made for ease of use over control of the updating process, when calling lookup.close() it will drop the reader object (relevant code).

So no, you can't continue to use lookup.get() or any maxmind-reader specific function after .close().

If you need more control you can directly use the internal geolite2.UpdateSubscriber class as mentioned in the docs. Please keep in mind that if you don't have automatic updates in place you are in breach of the license...

A few notes about self-updating:

If this isn't viable for your use case, we should try to work out a solution in the lib first.

AnandChowdhary commented 4 years ago

Basically, I saw this in my console once yesterday and also today:

events.js:282
      throw er; // Unhandled 'error' event
      ^

Error: getaddrinfo ENOTFOUND raw.githubusercontent.com
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:64:26)
Emitted 'error' event on ClientRequest instance at:
    at TLSSocket.socketErrorListener (_http_client.js:423:9)
    at TLSSocket.emit (events.js:305:20)
    at emitErrorNT (internal/streams/destroy.js:84:8)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: 'raw.githubusercontent.com'
}

And I have no idea how to debug it. There's no IP whitelisting, no firewall, etc., and I'm able to make a request to raw.githubusercontent.com with no problem using curl, also within Node.

I thought perhaps I can prevent installing updates, but you're right — I don't want to risk breaking the Terms of Use. Maybe a better way is to wrap the update function in a try { } catch and log if it was unable to update instead of throwing an error that might crash the app? Maybe an optional error handling configuration can be part of the package?

AnandChowdhary commented 4 years ago

Quick update: I'm able to see this message also when I'm doing jest --detectOpenHandles as part of my tests of the geolocation utility. This tells me that the update interval needs to be closed (probably using the close function you've built in) before ending the tests, using Jest's built-in afterAll()

Source:

All tests, however, are passing.

GitSquared commented 4 years ago

Maybe a better way is to wrap the update function in a try { } catch and log if it was unable to update instead of throwing an error that might crash the app?

Indeed, no error should bubble up to the outside app. I'll investigate this... The error originates from within node's std, are you sure there wasn't any connection / DNS problems?

This tells me that the update interval needs to be closed (probably using the close function you've built in) before ending the tests

Indeed, otherwise the test framework will detect it as a "leak". See this lib's own tests.

GitSquared commented 4 years ago

I released v1.0.4 with the above fix :arrow_up: