Avoid write on closed output writer

jmxtrans / embedded-jmxtrans

In process JMX metrics exporter.

MIT License

82 stars 37 forks source link

Avoid write on closed output writer #94

Closed YannRobert closed 9 years ago

YannRobert commented 9 years ago

When an application is stopped, the ApplicationContext stops all of the OutputWriters. Export tasks that were already scheduled or running then was trying to export to OutputWriters that were closed. This could produce some errors (alarming Exceptions in the logs), depending on the implementation of the OutputWriters. For instance, the GraphiteWriter closes it's internal connection pool when stopped, so that it cannot be used after that. Some others OutputWriters are fine with that because there stop method does nothing. (ConsoleWriter or GraphiteHttpWriter for instance) This PR introduces a state management class that ensures that a stopped OutputWriter is never written. Concurrent writes (to same OutputWriter by multiple Threads) are still supported.

buildhive commented 9 years ago

jmxtrans project » embedded-jmxtrans #109 SUCCESS This pull request looks good (what's this?)

YannRobert commented 9 years ago

The return type of EmbeddedJmxTrans.getOutputWriters() was changed from List<OutputWriter> to OutputWriterSet.

This may break for people that are using embedded-jmxtrans programmatically and using this method in their code. (if any?) Released version should increment minor number, or I can make the code compatible if needed.

YannRobert commented 9 years ago

hi @cyrille-leclerc Did you have time to look at this change proposal? Could you share your thoughts on this?

cyrille-leclerc commented 9 years ago

Hello @YannRobert,

I am sorry but I am not at ease with the OutputWriterSet which mixes the feature of a general purpose collection with things that are specific to embedded-jmxtrans and adds an outputWritersWithState list that is very complex to understand.

Could we stop exporting metrics as soon as stop() is invoked on a writer?
Could we add OutputWriter.isStopped() so that embedded-jmxtrans would not try to give result to a stopped writer?
Could we ensure that gracefulStop() ensure that all the ongoing collections of metrics are done before invoking stop()on any writer?

Did you face a special problem?

Cyrille

YannRobert commented 9 years ago

Just updated the PR branch with the changes from master