search

uber-archive / node-statsd-client

Node.js client for statsd
ISC License
28 stars 9 forks source link

Fix memory leak. Only send if sendQueue is small. #2

Closed Raynos closed 9 years ago

Raynos commented 9 years ago

There's a known unbounded queue in node core. Here we work around it and only send to the dgram Socket if the queue is small.

cc @jcorbin @rrix Would appreciate review

cc @mranney I set the default highWaterMark to 100, do you have a suggestion for a good sensible default.

Matt-Esch commented 9 years ago

This isn't a bad idea to prevent runaway. The highWaterMark can probably be much higher since we are just trying to limit the max memory used by the socket. Also this kind of limit probably behaves unwell at the edges. I want to know the following things in production:

To address the first point, we need some (rate limited?) event to listen on. The second could be addressed by emitting the last write delta with this event (how long has it been since I could last push into the queue).

Also in a scenario where we are simply pumping too much data through the statsd client, a more even throttle might be worth considering, i.e. testing how much data you can push through the client and evenly dropping packets. Putting a ceiling on it and warning when we hit that ceiling would be a great first pass though.

Raynos commented 9 years ago

@Matt-Esch this queue is only allocated when we are in a certain state of the dgram socket.

specifically it's a sendQueue whilst we are opening the socket. The only reason for this queue to grow is the socket emitted an error whilst opening and you ignored said error and keep sending into it.

Raynos commented 9 years ago

Managing my branches and pull requests -.-.

Closing this one in favor of https://github.com/uber/node-statsd-client/pull/3