Implemented close method to release scheduler resources on error

[andsel]

Close method is used to clean in case of error in run phase of the plugin, implemented to avoid leak of threads generated by Rufus scheduler.

Fixes #21

[kares]

👍 this makes sense - we actually have been seeing stuck threads from jdbc plugin.

[yaauie]

The code-change looks sensible.

I'd like to see a small change to the spec to eliminate the monkey-patch introduced here.

We also need a version bump (patch-level is okay) and an entry in the changelog, maybe something like:

Fixed potential resource leak by ensuring scheduler is shut down when a pipeline containing this plugin is shut down or restarted

[yaauie]

I'd rather avoid monkey-patching the plugin in our specs, since this modification to the class will persist beyond the scope of this individual spec.

An alternate solution would be:

Suggested change

	it "should cleanup resources on close" do
	class LogStash::Inputs::Jdbc
	def scheduler_down?
	@scheduler.down?
	end
	end
	runner = Thread.new do
	plugin.run(queue)
	end
	sleep 1
	plugin.do_close
	expect(plugin.scheduler_down?).to be_truthy
	end
	it 'cleans up scheduler resources on close' do
	runner = Thread.new do
	plugin.run(queue)
	end
	sleep 1
	plugin.do_close
	scheduler = plugin.instance_variable_get(:@scheduler)
	expect(scheduler).to_not be_nil
	expect(scheduler.down?).to be_truthy
	end

[andsel]

Thank's @yaauie good point, I missed it!

[andsel]

@yaauie I've added the suggested changes, if you want you can go forward with the review

[andsel]

Jenkins test this please

[yaauie]

LGTM 👍

[elasticsearch-bot]

Andrea Selva merged this to master!