Skip to content
This repository was archived by the owner on Jan 23, 2020. It is now read-only.

Stop statsd client after sending data#73

Merged
JohnLZeller merged 1 commit into
DataDog:masterfrom
suxor42:oom-fix
Jan 26, 2017
Merged

Stop statsd client after sending data#73
JohnLZeller merged 1 commit into
DataDog:masterfrom
suxor42:oom-fix

Conversation

@suxor42

@suxor42 suxor42 commented Dec 13, 2016

Copy link
Copy Markdown

The current implementation creates a new DescriptorImpl instance for every call
to onCompleted. For every new instance a new statsd client is created.
The statsd client is never stopped. This causes a resource(thread) leak.
The resource leak will cause a java.lang.OutOfMemoryError: Unable to create new native thread
How fast this happens depends on how often jobs are executed and the OS
https://plumbr.eu/outofmemoryerror/unable-to-create-new-native-thread#example

This PR stops the statsd client after it sent the data. A better fix would be to implement
DescriptorImpl as a real singleton

The current implementation creates a new DescriptorImpl instance for every call
to onCompleted. For every new instance a new statsd client is created.
The statsd client is never stopped. This causes a resource(thread) leak.
The resource leak will cause a `java.lang.OutOfMemoryError: Unable to create new native thread`
How fast this happens depends on how often jobs are executed and the OS
https://plumbr.eu/outofmemoryerror/unable-to-create-new-native-thread#example

This PR stops the statsd client after it sent the data. A better fix would be to implement
DescriptorImpl as a real singleton
@alexouzounis

Copy link
Copy Markdown

:+1 being hit by this, makes the plugin unusable

@jshirley

Copy link
Copy Markdown

Same :( -- @JohnLZeller do you have any idea when this could be merged and/or if there is more we should do here?

@jshirley-stripe

Copy link
Copy Markdown

Bump!

@irabinovitch

Copy link
Copy Markdown

@jshirley Thank you for following up. We are moving closer to a release, as we have several PRs open. Let me circle up with @JohnLZeller and see if we can offer a specific timeline.

@JohnLZeller

Copy link
Copy Markdown
Contributor

Thanks for your contribution @suxor42! This looks good as is. It'll be in the next release

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants